Skip to content

ir!: the validation constraints WA Web enforces, not just field shapes (schemaVersion 2.0.0) - #36

Merged
jlucaso1 merged 37 commits into
mainfrom
ir-validation-constraints
Jul 28, 2026
Merged

ir!: the validation constraints WA Web enforces, not just field shapes (schemaVersion 2.0.0)#36
jlucaso1 merged 37 commits into
mainfrom
ir-validation-constraints

Conversation

@jlucaso1

@jlucaso1 jlucaso1 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Contract break — schemaVersion 1.0.0 → 2.0.0. Migration at the bottom; four mechanical changes for a 1.x consumer.

Summary

The IR describes what fields a stanza has. That is enough to read a well-formed stanza and not enough to produce one, or to know why a real client rejected the one you produced. This adds the rules WA Web's own parsers enforce — symmetric by construction, so they serve a parser and an emitter equally.

Constraint Where in the IR Recovered
Echo rules — an answer's from must equal the request's to assertions[].kind == "reference" + referencePath; referencePath on a field for the optional form 584
Pinned valuestype="admin", matched="true", code=429 literalValue on ParsedField 1784
Per-RPC error vocabulary — a closed, per-RPC set errorArms (+ errorEnvelope) and kind on a variant 645 arms, 104 side-typed
Response enums — the legal values, not a bare "type": "enum" enumRef / enumKeys on ParsedField 151
Notification action unions — the payload inside the envelope notifications[].actions, and NOTIF_ACTIONS in the reference notif.rs 47 arms, 184 shape elements
Typed content leaves — an integer/enum/byte-range element body, not a coarse contentType type + byteLength/byteMin-byteMax/literalValue on a content field PreKeyBundle <registration> (4 bytes) <type> (1) <id> (3); typeElementValue pinned to 05

Why

Each of these has cost a consumer real debugging time:

  • build_iq_error hardcoded from="s.whatsapp.net", so every error answering a request addressed to g.us was unparseable — the client raised SmaxParsingFailure instead of surfacing the error. ~50 error sites, mis-filed for months as "the mock does not implement X".
  • A successful promote was answered <participant type="200"> — a status code where the role goes — and the client rejected the whole response of an operation that had already been applied.
  • An empty batch was answered 404 item-not-found, which matches no branch of that RPC. The vocabulary is per-RPC and there was no way to know it except reading each parser.
  • The disappearing-message timer was read as ephemeralDuration (the create payload's spelling) when the action field is duration — every timer reported as 0. And not_ephemeral normalises into ephemeral with duration: 0, so a NOT_EPHEMERAL branch is dead code.

Answerable from the IR alone

// 1. "For RPC X, what must an error response's `from` be?"  → the request's `to`.
{ "kind": "reference", "name": "from", "referencePath": ["to"] }

// 2. "What does BatchGetGroupInfo accept?"  → a closed set of PAIRS; note no 404.
{ "tag": "BatchGetGroupInfoResponseClientError", "kind": "client_error",
  "errorArms": [ { "name": "IQErrorBadRequest",    "code": 400, "text": "bad-request" },
                 { "name": "IQErrorRateOverlimit", "code": 429, "text": "rate-overlimit" } ] }

// 3. "What pins a successful promote?"  → optional type="admin".
{ "method": "maybeAttrString", "name": "type", "wireName": "type",
  "literalValue": "admin", "required": false }

// 4. "Which wire tags map to group action `ephemeral`, and what does each set?"
{ "wireTag": "ephemeral",     "actionType": "ephemeral",
  "fields": [{ "name": "duration", "wireName": "expiration", "type": "integer" }] }
{ "wireTag": "not_ephemeral", "actionType": "ephemeral",
  "constantFields": [{ "name": "duration", "value": 0 }] }

Design notes

The error vocabulary is pairs, not two lists. errorCodes: [400, 501] beside errorTexts: ["bad-request", "feature-not-implemented"] admits four combinations where the parser accepts two — an emitter picking one from each list sends 400 feature-not-implemented, unparseable. 117 variants were ambiguous that way. errorArms pairs them, and the flat lists were removed rather than kept as a convenience view: a convenient view of an unsound shape is still unsound.

A two-level error carries an envelope. SetResponsePreKeySuccessVnameFailure wraps <error code="207"> around an inner <error> whose text the disjunction pins. Pins are partitioned by the node they are read from, so the envelope holds only the outer node's and the inner node's shared range stays on the arms — mixing them would say code 207 must also fall in 400–599.

The error side comes from the codes, not the name. WA spells the arms inconsistently, and SetResponsePreKeySuccessVnameFailure reads as a success while asserting type="error". Classification takes the parser's root discriminator first, then refines by code range. A variant whose codes settle nothing stays a plain error (45 of them) rather than being guessed into a side, and an error-ish name on a parser that asserts type="result" is a structured non-happy outcome (alternative, 2 of them) — never an error, because the wire discriminator outranks the name in both directions.

Required and optional pins are different constraints. A literal is a hard discriminator — present and exact or the variant does not match. An optionalLiteral guards nothing, so it records no assertion; its value still rides on the field. On a Bool presence flag the pin is always conditional: required: true describes the flag the parser always produces, never the wire attribute it reports on.

The action union resolves through the handler's own helpers. The w:gp2 arms delegate to module-local functions and the minifier hoists nearly every accessor into a local, so the reader inlines helpers (bounded), follows var bindings in statement order, and builds a scope per return path — sibling branches routinely rebind the same minified name to different accessors. A branch-only field is optional; a key bound to different wire reads in two branches is dropped rather than resolved to the first.

Missing beats wrong, consistently. Ambiguity is refused rather than guessed — a constant table bound twice, a helper name reused by a nested function, a child mapped under one name with two wire tags. Anything seen but not structurally resolvable is counted under dropsByReason, keyed per parser site, so "no constraint here" and "a constraint we failed to extract" never look alike.

Guards

  • Round-trip (crates/wa-scan/tests/iq_roundtrip.rs) — for every IQ success shape, build a stanza from the IR alone and check the recorded constraints accept it. 140 shapes, 118 constrained, 180 nested pins exercised, against a group-addressed request so a hardcoded s.whatsapp.net fails. It proves the constraints are mutually satisfiable and correctly placed — not that none is missing, since emitter and checker read the same list; the module doc says so.
  • Named canaries — the promote type="admin", both blocklist matched pins, and a floor on the fromto echo. An aggregate stays healthy while one variant quietly loses one pin; a named assertion cannot.
  • Floor guarddiagnostics.iq.constraints and diagnostics.notif.{actions,actionShapes} are regression-checked; the counters may not shrink without --allow-shrink. It fired four times during review: three legitimate reductions (de-duplicated counters, phantom account_sync arms) and once on a real regression of mine, before it reached generated/.
  • Per-domain drop countsdropsByReason now exists for incoming and notif as well as iq. A constraint seen and not recovered is counted in the domain whose artifact it belongs to, so the number describes the file beside it: notif reports the one action enum table (create.reason) it cannot name, incoming reports 0.

Migration from 1.x

  1. AssertionKind gained reference — handle or ignore it. (A closed-enum consumer rejects the document: validating the current iq/index.json against the 1.0 schema fails 579 times.)
  2. ResponseVariantKind gained client_error / server_error — a variant that was error may now be either. Match all three, or use is_error().
  3. A variant's errorCodes / errorTexts / errorCodeMin / errorCodeMax / errorClass are goneerrorArms (+ errorEnvelope).
  4. ContentType gained integer — a <registration> whose body is a number used to be reported as string.

Also non-breaking but worth knowing: enum accessors now decode to type: "enum" rather than "string" (486 fields already did; 28 disagreed), and maybeAttrX derives its type from attrX, which fixed a PN user JID reported as a bare string.

Review

Thirty-one rounds, 173 findings from Codex (153) and CodeRabbit (20), all applied or answered with evidence; 170 threads resolved plus 3 raised in review bodies, one declined on the record (see below).

Several corrected wrong values already committed, and a majority of the later rounds were regressions introduced by earlier fixes in the same review — the recurring shape being a predicate that enumerated accessor spellings instead of deriving them from wap. That bug was found at five separate sites; the fifth is why the "byte-identical codegen" line below now reads differently.

One finding was declined on the record — constraint enforcement in the generated parsers; see the follow-ups below.

Follow-ups, deliberately not in this PR

Each of these came up during the work or the review, was measured rather than guessed, and is separable. None blocks this change.

1. Enforce the constraints in the reference codegen. This PR makes the constraints expressible; nothing consumes them yet. The generated parser decodes and validates nothing at all: 2178 literalValue, 169 intMin, 88 byteLength, 108 byteMin, 281 enumRef all go unchecked, and assertions are used only to dispatch a union, never to reject. Codex asked for byte-content validation specifically; enforcing only the newest constraint kind would leave a consumer rejecting an out-of-range byte payload while still accepting code=999 on a field pinned to 429. The design question is the surface — a validate() pass, a Result from the parser, typed errors — and it should be answered once for every kind.

2. Compile the generated Rust in CI. generated/**/*.rs is gitignored and CI only checks that it parses (syn::parse_file). Building it against a real wacore is what catches type errors, and it has: a smoke crate found 133 of them in an earlier round. Two of this PR's own bugs are the argument: the contentUint fields reached generated code, parsed fine as Rust, and decoded binary as decimal text; and an optional JID would have emitted a Jid initializer for an Option<Jid> field — syntactically valid, type-incorrect, and latent only because no such field exists yet. A tiny throwaway crate in CI would close both.

3. previousDocIds — the mex doc-id history. WA rotates a mex operation's docId and the current artifact can only ever show the newest. Measured across the 32 committed versions of generated/mex/index.json: 139 operations, 15 have rotated, 20 retired doc ids are recoverable from a one-shot backfill of git history. It cannot live in mex/index.json — that file must stay a pure function of the pinned bundles for the determinism gate — so it needs a separate accumulator outside the reproducibility chain. wasm.lock.json is the precedent. Shape: operationName → { docId → {firstSeen, lastSeen} }.

4. Pin the bootloader maps. The extracted structured data (rsrcMap / compMap / bxData / consistency.rev) is what the discovery path actually depends on, and none of it is pinned today. Pin the extraction, not the raw HTML — nonces and timestamps make the page byte-unstable, so hashing it would fail the determinism gate for reasons that have nothing to do with the protocol.

5. A real dataflow pass for the action reader, or a documented scope boundary. actions.rs has grown a hand-rolled JS scope/control-flow analysis, and the last six rounds of review have been a stream of constructs it does not model (do…while breaks, finally writes, for initializers, conditional-expression writes, object spreads, by-name callbacks, accessor aliases). Three are still open, recorded on their threads. generated/notif/index.json has not moved across those six rounds, and patching construct-by-construct has itself introduced regressions in five of them — so the class wants a real pass, or an explicit statement of what the reader does and does not model, rather than another round.

6. The constraints that are counted but still lost. dropsByReason now names them rather than hiding them, which is the point, but they are real gaps: 37 IQ enum tables WA composes at runtime (babelHelpers.extends({}, base, {error: "error"})), and 1 w:gp2 action table (create.reason) that is a module-local object rather than a nameable export. Both need a small evaluator rather than a structural matcher.

Validation

  • cargo fmt --all --check; clippy --workspace --all-targets and --all-features --all-targets, both -D warnings

  • cargo test --workspace571 tests, 119 added here; cargo test -p wa-ir --features schema

  • cargo check -p wa-ir --target wasm32-unknown-unknown; cargo build -p whatspec --no-default-features

  • scripts/validate-schemas.py generated → 12/12

  • determinism: update --bundles <pinned> --check27 committed artifact(s) up to date

  • the reference Rust codegen gains 26 lines over main (23 071 → 23 097), verified by regenerating from both and diffing

    For most of this review that line read "byte-identical", and that was a bug reporting itself as good news: wa-codegen still enumerated three content spellings, so the contentUint fields the scanners recovered were carried in the IR and dropped at generation. The unchanged output was the symptom. The added lines are those fields — registration: u64, r#type: u64, id: u64 and their reads — now reaching generated code.

    Two rounds later a third defect surfaced in the same file: 108 JID fields the IR marks required: false were declared Jid and parsed with ok_or_else("missing …"), because optionality was derived from the accessor spelling rather than from the data. The generated parser rejected responses the IR says may omit them.

    And one round after the fields arrived they were still decoded wrong: contentUint(N) is N big-endian bytes, not decimal text (contentInt is the text one), so the first version of those reads ran content_str().parse() over binary and yielded 0 every time. They decode from content_bytes() now, and byteLength records the width.

Summary by CodeRabbit

  • New Features
    • Added richer notification payload extraction with per-tag action unions, constants, nested children mapping, and improved optional/required recovery.
    • Added expanded IQ constraint validation, including echoed request-value rules and structured typed error/outcome modeling.
    • Introduced deeper diagnostics and coverage reporting for both IQ constraints and notification action-union extraction.
  • Bug Fixes
    • Improved field type detection across additional accessor spellings (enums, integers, bytes, content, and JIDs).
    • Ensured unsupported/ambiguous constraints are tracked as dropped rather than silently approximated.
  • Documentation
    • Updated validation-constraints and cache filename/integrity guidance.
  • Tests
    • Added/expanded regression and roundtrip validation to enforce the new constraint and notification metrics.

…d shapes

The IR described what fields a stanza has. That is enough to read a
well-formed stanza and not enough to produce one, or to know why a real
client rejected the one you produced. Every rule below is symmetric — it
serves a parser and an emitter equally — and each one has cost a consumer
real debugging time.

Additive and schema-versioned (schemaVersion 1.0.0 -> 1.1.0): every new
field is optional, no existing field changes shape or value, and the whole
generated/ diff is verified strictly additive (nothing removed anywhere,
including assertion arrays).

1. Echo rules. A new `reference` assertion kind carrying `referencePath`,
   the argument list of WA's `attrStringFromReference`: an answer's `from`
   must equal the REQUEST's `to`, its `id` the request's `id`. 17 of the 26
   namespace error mixins enforce it, and so does every success parser. An
   emitter that hardcodes from="s.whatsapp.net" makes every answer to a
   g.us request unparseable. 579 recovered — including the <ack> shapes in
   the incoming domain, which share the analyzer.

2. Pinned field values. `literalValue` on ParsedField, from both `literal`
   (required, a hard discriminator) and `optionalLiteral` (pinned only when
   present) — type="admin" on a successful promote, matched="true"/"false"
   on a blocklist update, code=429 on a rate-limit error. `required` tells
   the two apart, which is what decides whether an emitter MUST send the
   attribute or merely must not contradict it. An optional echo, which
   guards nothing, rides on the field as `referencePath` instead. Numeric
   literals are now captured too (they were silently dropped). 2174 pins.

3. Per-RPC error vocabulary. `errorClass` (client/server, derived from the
   codes, not from the variant's inconsistently-spelled name) plus the
   closed `errorCodes`/`errorTexts` sets and the `errorCodeMin`/`Max` range
   of the open-ended fallback arms. The vocabulary is per-RPC:
   BatchGetGroupInfo takes 400/429 and rejects 404 item-not-found, though
   that mixin exists and other RPCs use it. `kind` is deliberately left
   alone so existing consumers keep matching on success/error.

4. Response enums. `enumRef` on ParsedField, resolved through the same
   path the request side already uses, so an `attrStringEnum` field carries
   its legal values instead of a bare "type": "enum". 277 resolved.

5. Notification action unions. A new `actions` list per notification: the
   payload inside the envelope, keyed by child tag. The wireTag ->
   actionType mapping is many-to-one (not_ephemeral normalises into
   ephemeral with duration 0, so a NOT_EPHEMERAL branch is dead code) and
   field names are rebound (the timer arrives in `expiration`, the action
   field is `duration`). Neither is derivable from the wire. 55 arms, 45 of
   them w:gp2, with fields, constants, and repeated sub-elements resolved
   through the handler's local helpers.

Guards:

- A round-trip test (crates/wa-scan/tests/iq_roundtrip.rs) builds a stanza
  for every IQ success shape from the IR alone and checks the recorded
  constraints accept it — 141 shapes, plus a negative control proving a
  hardcoded `from` is caught. Emitter and checker share no code with the
  extractor, so a mis-extraction cannot cancel itself out.
- manifest.diagnostics.iq.constraints and diagnostics.notif.actions are
  floor-guarded, so a WA refactor that hides one of these constructs fails
  the update instead of silently emptying a field.
- Constraints seen but not structurally resolvable are counted under
  dropsByReason, so "no constraint here" and "we failed to extract one"
  never look alike.

Determinism holds (update --check is clean from the pinned bundles) and
the reference Rust codegen is byte-identical, verified by A/B-ing it
against the same IR with the new fields stripped.
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR expands the IR with request-echo, literal, enum, and error-vocabulary metadata; extracts notification action unions; propagates constraint diagnostics; updates schema versioning, tests, documentation, cache semantics, and regression floor checks.

Changes

IR extraction and coverage

Layer / File(s) Summary
IR contracts and compatibility
crates/wa-ir/..., crates/wa-codegen/...
IR structures now represent reference paths, pinned values, enum references, error vocabularies, and notification actions; schema version and dependent fixtures are updated.
IQ constraint and error extraction
crates/wa-scan/...
IQ parsing recovers echoes, literals, enums, error classes, and accepted error values while tracking unresolved constraints and validating IR round trips.
Notification action-union extraction
crates/wa-notif/...
Notification handlers are analyzed for const-keyed switches, normalized action types, fields, constants, and mapped children, with synthetic bundle coverage.
Diagnostics and documentation
crates/whatspec/..., README.md, crates/wa-fetch/...
Constraint drops and coverage counters are propagated into manifests and floor checks; documentation distinguishes URL-based cache labels from content hashes.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Bundle
  participant wa_scan
  participant wa_notif
  participant whatspec
  Bundle->>wa_scan: extract IQ fields and constraints
  Bundle->>wa_notif: extract notification action unions
  wa_scan->>whatspec: return IQ diagnostics and drop counts
  wa_notif->>whatspec: return notification action counts
  whatspec->>whatspec: write manifest diagnostics and check floors
Loading

Possibly related PRs

Suggested reviewers: greptile-apps

Poem

I’m a bunny with constraints in my den,
Echoes and enums now make sense again.
Actions hop from tags in a row,
Diagnostics tell what parsers know.
The manifest blooms—then floors guard the way!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main shift from field shapes to validation constraints and the schema version bump.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 27, 2026

Copy link
Copy Markdown

Greptile Summary

The PR expands the IR to schema version 2.0.0 with parser-enforced validation constraints and notification action unions.

  • Adds request-reference assertions, literal values, enum references, and structured per-RPC error vocabularies.
  • Adds notification action extraction with branch-aware field, child, constant, and requiredness recovery.
  • Extends diagnostics, generated schemas, generated artifacts, and regression coverage for the new constraints.
  • Updates documentation and consumers for the breaking schema contract.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
crates/wa-ir/src/iq.rs Extends the IQ model with reference assertions, literal and enum constraints, structured error arms, and richer response variant classification.
crates/wa-scan/src/response_smax.rs Expands smax response analysis to recover references, pinned values, enum links, ranges, and nested parser constraints.
crates/wa-scan/src/response_index.rs Builds structured error vocabularies and classifies response variants from recovered parser constraints.
crates/wa-notif/src/actions.rs Introduces branch- and scope-aware extraction of notification action unions, fields, children, and constants.
crates/wa-notif/src/lib.rs Integrates action extraction and response enum linking into notification IR generation.
crates/whatspec/src/main.rs Propagates the schema-v2 artifacts and adds diagnostics and regression floors for IQ constraints and notification actions.
generated/iq/index.json Regenerates the IQ contract with reference assertions, literal constraints, enum references, and paired error arms.
generated/notif/index.json Regenerates notification definitions with extracted payload action unions and normalized fields.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[WhatsApp Web bundle] --> B[IQ and response scanners]
    A --> C[Notification action scanner]
    B --> D[Constraint and enum linking]
    C --> E[Action union resolution]
    D --> F[Schema v2 IR]
    E --> F
    F --> G[Generated JSON and schemas]
    F --> H[Reference code generation]
    D --> I[Constraint diagnostics]
    E --> I
    I --> J[Manifest floor guards]
Loading

Reviews (18): Last reviewed commit: "scan: count what the non-IQ domains lose..." | Re-trigger Greptile

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/wa-notif/src/actions.rs`:
- Around line 659-691: Cap recursive descent in find_accessor with a depth
budget consistent with the module’s existing MAX_INLINE_DEPTH approach,
decrementing it whenever inspecting nested wrapper-call arguments and returning
None when exhausted. Preserve direct accessor detection and ensure cyclic scope
bindings terminate without stack overflow.

In `@crates/wa-notif/src/tests.rs`:
- Around line 732-741: The test
handler_without_a_const_keyed_switch_has_no_actions is vacuous because
GROUP_ACTIONS_BUNDLE contains only w:gp2. Extend the fixture or test input with
a second notification whose handler uses plain WADeprecatedWapParser or a
non-const-keyed switch, then retain the assertion that non-w:gp2 notifications
have no actions.

In `@crates/wa-scan/src/response_index.rs`:
- Around line 137-154: Compute error_vocabulary(&fields) only when kind is
ResponseVariantKind::Error, and otherwise use the existing empty/default
vocabulary values for error_codes, error_texts, error_code_min, and
error_code_max. Update the ResponseVariant construction in the surrounding
variant-indexing flow while preserving error_class behavior and the
per-error-arm semantics.

In `@crates/wa-scan/src/response_smax.rs`:
- Around line 1376-1393: Preserve request echo provenance in the
`Binding::Reference` arm by setting `ParsedField.reference_path` to a clone of
`path` alongside `wire_name`. Update `sample_request()` and related IQ roundtrip
handling so every newly surfaced reference path is modeled, avoiding `unmodelled
request path` failures while keeping `iq_constraint_counts` able to count these
fields.
- Around line 120-152: Update resolve_enum so every unresolved enum occurrence
increments the response-enum drop counter, including cache hits; perform the
drop_note accounting before returning a cached unresolved result or otherwise
separate memoization from per-occurrence counting. Preserve caching and avoid
counting successfully resolved enums.
- Around line 625-653: Update the optionalLiteral branch to derive method and
field_type from its wrapped accessor using inner_accessor_name and
normalize_accessor, matching the literal branch; retain the existing string
accessor as the fallback when the wrapped accessor cannot be resolved. Preserve
the literal_value, reference_path, and optional-field behavior while ensuring
integer, JID, and bytes pins carry their correct parsed type.
- Around line 817-842: Update enum_arg_ref to record a drop in dropsByReason
when an enum accessor has a valid attrStringEnum or contentStringEnum accessor
but module_member_ref cannot identify an eligible enum reference. Preserve the
existing resolver path for recognized o("Mod").NAME references, and ensure
inline objects, aliases, and WASmaxParse-owned references are reported before
returning None.

In `@crates/wa-scan/tests/iq_roundtrip.rs`:
- Around line 239-249: Extend the IQ round-trip coverage tracking near checked
and failures to count shapes containing a Reference assertion or pinned-field
constraint, then add an assertion requiring that count to be greater than zero.
Keep the existing checked and failures assertions unchanged, and report the new
coverage metric only as needed to make the floor diagnostic.

In `@crates/whatspec/src/main.rs`:
- Around line 826-871: Update iq_constraint_counts so the primary Success
variant’s fields are traversed only once: either count the shared
s.response.fields representation and skip that corresponding variant, or count
variants while excluding the primary Success variant. Preserve counting of
assertions, typed errors, error texts, and fallback responses with empty
variants, and ensure field_literals, reference_constraints, and field_enum_refs
are not duplicated.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: eb4f64bd-b1ec-430e-88cf-5e8699744d8d

📥 Commits

Reviewing files that changed from the base of the PR and between 096e956 and ced8322.

⛔ Files ignored due to path filters (19)
  • Cargo.lock is excluded by !**/*.lock
  • generated/abprops/index.json is excluded by !**/generated/**
  • generated/appstate/index.json is excluded by !**/generated/**
  • generated/enums/index.json is excluded by !**/generated/**
  • generated/incoming/index.json is excluded by !**/generated/**
  • generated/iq/index.json is excluded by !**/generated/**
  • generated/manifest.json is excluded by !**/generated/**
  • generated/mex/index.json is excluded by !**/generated/**
  • generated/notif/index.json is excluded by !**/generated/**
  • generated/schema/incoming.schema.json is excluded by !**/generated/**
  • generated/schema/iq.schema.json is excluded by !**/generated/**
  • generated/schema/notif.schema.json is excluded by !**/generated/**
  • generated/schema/srvreq.schema.json is excluded by !**/generated/**
  • generated/schema/stanza.schema.json is excluded by !**/generated/**
  • generated/srvreq/index.json is excluded by !**/generated/**
  • generated/stanza/index.json is excluded by !**/generated/**
  • generated/tokens/index.json is excluded by !**/generated/**
  • generated/wam/index.json is excluded by !**/generated/**
  • generated/wasm/index.json is excluded by !**/generated/**
📒 Files selected for processing (17)
  • README.md
  • crates/wa-codegen/src/notif_export.rs
  • crates/wa-codegen/src/spec.rs
  • crates/wa-ir/src/iq.rs
  • crates/wa-ir/src/lib.rs
  • crates/wa-ir/src/notif.rs
  • crates/wa-notif/Cargo.toml
  • crates/wa-notif/src/actions.rs
  • crates/wa-notif/src/lib.rs
  • crates/wa-notif/src/tests.rs
  • crates/wa-scan/Cargo.toml
  • crates/wa-scan/src/lib.rs
  • crates/wa-scan/src/response.rs
  • crates/wa-scan/src/response_index.rs
  • crates/wa-scan/src/response_smax.rs
  • crates/wa-scan/tests/iq_roundtrip.rs
  • crates/whatspec/src/main.rs

Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-notif/src/tests.rs
Comment thread crates/wa-scan/src/response_index.rs
Comment thread crates/wa-scan/src/response_smax.rs Outdated
Comment thread crates/wa-scan/src/response_smax.rs
Comment thread crates/wa-scan/src/response_smax.rs
Comment thread crates/wa-scan/src/response_smax.rs
Comment thread crates/wa-scan/tests/iq_roundtrip.rs Outdated
Comment thread crates/whatspec/src/main.rs

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ced832234c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-notif/src/actions.rs
Comment thread crates/wa-scan/src/response_smax.rs
Comment thread crates/wa-notif/src/lib.rs
Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-scan/src/response_index.rs Outdated
Comment thread crates/wa-notif/src/actions.rs
Comment thread crates/wa-scan/tests/iq_roundtrip.rs Outdated
`referencePath` is documented as relative to the request, and a consumer
acts on that. The extractor keyed only on the binding being a
`WASmaxParseReference` helper, so it was reporting an observed property as
if it were a guaranteed one: if WA ever pointed one of those helpers at a
node other than the parser's `reference` parameter, the IR would still say
"the request" and every consumer would be silently wrong.

The reference argument is now checked against the parser's second
parameter and rejected otherwise, with the rejection counted under
dropsByReason rather than dropped in silence.

The invariant already held: coverage is unchanged at 579 reference
constraints, no new drop reason appears, and generated/ is byte-identical.
That is the point — it is now enforced instead of assumed.

Also groups the per-function analysis context (locals, resolver, reference
parameter) into `FnCtx` so the classifier keeps a readable signature.

docs: a cache filename is a location label, not a content address

The durable release store is content-addressed (`bundles-<version>-<setHash>`),
the cache is not: a JS entry is named after the URL's last segment and a
wasm entry after `sha256(url)`, while the hash of the *bytes* lives in the
cache manifest and is what integrity is verified against. Two hashes of
different things sat three lines apart in the README with nothing saying so,
which is enough to cost a reader time even though no sentence was false.
@jlucaso1

Copy link
Copy Markdown
Contributor Author

Review round

Checked the three points against the bundle. Two turn out to be unfounded, and I have the evidence rather than an argument; the third was real and is fixed in 5500115.

1. literalValue inflating into false constraints — cannot happen. There is no "compare without pinning" form in the vocabulary. WASmaxParseUtils, deminified:

// literal(accessor, node, name, expected)
function T(e,t,n,r){ var a=e(t,n); return a.success ? (a.value===r ? makeResult(r)
  : O(t,'to have "'+n+'"={'+r+'}, but instead has "'+a.value+'"')) : a }

// optionalLiteral(accessor, node, name, expected)
function I(t,n,r,a){ if(a==null) return voidSuccess; var i=k(t,n,r);
  return i.success ? (i.value===a ? makeResult(a)
                    : i.value==null ? voidSuccess          // absent is fine
                    : O(n,'to have "'+r+'"={'+a+'}…')) : i }

Both fail the parse on a mismatch; they differ only on absence — which is exactly the required split the IR records. So a captured literalValue is always a rule the client enforces.

The ratio is the right shape too. Overall 1779/4119 (43%), but that is dominated by error variants, where text + code are pinned by construction — that is the vocabulary. Success variants are sparse and the pins are precisely the discriminators:

variant pinned/total what is pinned
BatchGetGroupInfoResponseSuccess 10/157 can_auto_file=true, default_membership_approval_mode=request_required, …
PromoteDemoteResponseSuccessPromote 2/11 type=result, type=admin
PromoteDemoteResponseSuccessDemote 1/10 type=result
BatchGetGroupInfoResponseClientError 5/6 text/code pairs + type=error

Note SuccessDemote has no type=admin — the pin is on promote only, which is the asymmetry an emitter needs.

2. errorCodeMax is extracted, never synthesised. Exhaustive grep of every code range in the bundle — all eight sites are two-sided:

2  attrIntRange(N,"code",400,499)
2  attrIntRange(N,"code",400,599)
4  attrIntRange(N,"code",500,599)

IQErrorFallbackServer is attrIntRange(e,"code",500,599), so errorCodeMax: 599 is the parser's own ceiling. The only unbounded-looking code>=500 in the bundle is a.code>=500&&a.code<600 in a stream-failure handler (WAComms.stopComms / startLogin), not an IQ error parser — and still bounded. The IR emits exactly two ranges, (500,599) ×36 and (400,499) ×27.

Worth noting the (400,599) sites: those span both families, and error_class() returns None for them rather than picking one — no fabricated classification.

3. Real, and now enforced. referencePath was documented as relative to the request but the extractor only keyed on the binding being a WASmaxParseReference helper — it reported an observed property as a guaranteed one. Probed all 260 call sites in the bundle: 224 pass the function's 2nd parameter (the request), 36 pass the 1st — and those 36 are single-parameter function e(e) builders in WASmaxOut* modules, which this analyzer never reads.

So the invariant held, but nothing enforced it. 5500115 checks the reference argument against the parser's reference parameter and rejects otherwise, counting the rejection under dropsByReason. Coverage is unchanged (579 constraints, no new drop reason) and generated/ is byte-identical — which is the point: assumed → enforced, at zero cost. A regression test covers the rejection path.

The emitted path vocabulary is small and all request-relative: ["id"] ×308, ["to"] ×283, ["type"] ×1.

Also in this round

crates/wa-fetch/src/cache.rs + README: the durable release store is content-addressed (bundles-<version>-<setHash>), the cache is not — a JS entry is named after the URL's last segment, a wasm entry after sha256(url), and the hash of the bytes lives in the cache manifest, which is what integrity is checked against. No sentence was false, but two hashes of different things sat three lines apart with nothing distinguishing them. Now stated explicitly in both places.

Separately: the overlong-filename concern is already fixed, by #31. disk_file_name bounds writes at MAX_BUNDLE_FILE_NAME = 128 with a URL-derived fallback, and Bundle.file_name's doc already warns that the name actually written may differ. Anything going through save_bundles is safe; only a consumer that materialises Bundle.file_name itself can still overflow, and that is the documented case.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 55001157e9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-scan/src/response_index.rs Outdated
Comment thread crates/wa-scan/src/response_index.rs Outdated
Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/whatspec/src/main.rs
Sixteen of the seventeen review findings applied; one rejected with
evidence. The four that changed committed output were all verified against
the bundle first.

Wrong data in the committed IR:

- `iq_constraint_counts` walked `response.fields` AND every variant, but
  `response.fields` is a *clone* of the primary success variant's fields
  whenever variants exist — so 97 of 143 stanzas had their constraints
  counted twice, and the inflation wasn't even uniform. fieldLiterals
  2174 -> 1782 and fieldEnumRefs 277 -> 146 are the corrected totals, not a
  regression: counting the OLD IR with the fixed logic gives exactly the
  same numbers. `--allow-shrink` was required once to move past the
  inflated floor.
- The error vocabulary was read only from fields inside a `UnionVariant`,
  but plenty of RPCs parse a single `<error>` child directly instead of an
  `errorMixinDisjunction`. 29 error variants silently had no vocabulary:
  `SetResponseConflict` (409/conflict), `…TooManyAttempts` (431), and so
  on. The code/text reading now applies to every traversed field.
- Conversely, the vocabulary was computed for EVERY variant kind and only
  `error_class` was gated on `Error`, so three PreKeys *success* variants
  carried a bogus accepted-error set. The whole computation is now gated.
- An optional echo consumed as a presence flag lost its rule entirely. The
  blocklist shapes bind `optionalLiteral(attrString, list, "c_dhash",
  ref.item.dhash)` and expose only `{hasListCDhash: m.value != null}`, so
  the pin was dropped with no field and no drop note — worse than before,
  since the previous behaviour at least counted it. The presence field now
  carries the pin (5 recovered; referenceConstraints 579 -> 584).

Latent correctness:

- `optionalLiteral` hardcoded `maybeAttrString`/String instead of deriving
  the type from its wrapped accessor, so an `optionalLiteral(attrInt, …,
  429)` pin would be mis-typed — and `field_type` is exactly what tells a
  consumer how to read `literalValue` back.
- A request-derived output field dropped its `path` after using the tail as
  `wire_name`, making it indistinguishable from a field read off the
  response.
- `find_accessor` recursed through `deref_ident` with no depth budget, so
  mutually referential minified bindings would blow the stack — a crash of
  the generator rather than a field that isn't recovered.
- A merged action branch only weakened fields it contained, leaving a field
  absent from that branch wrongly required.
- A hoisted content receiver (`var body = child.child("body")`) yielded an
  empty wire name; the receiver is now dereferenced through the scope.
- Returns nested in `if`/`else` were skipped, exposing only the last shape
  of a branching arm. Nested `switch`es are still not descended into —
  those are a different dispatch level, not branches of this arm.
- A constant-table local bound twice to different objects now resolves to
  nothing instead of last-one-wins, so a reused minified name cannot mint
  wrong wire tags.

Diagnostics:

- The enum drop counter was memoized, so it counted distinct enums rather
  than occurrences while every other reason here is per-occurrence (10 ->
  65), and an enum argument that wasn't an `o("Mod").NAME` reference at all
  was dropped with no note.

Tests:

- The round-trip guard modelled only the root, so it skipped every
  `sourcePath` and nested-child pin — including `matched="true"` on
  `<list>` and `type="admin"` on `<participant>`, i.e. it would have
  accepted the very malformed promote response its own header describes.
  It now models the stanza as a path-addressed tree: 175 nested pins
  exercised where there were none, plus a regression test that a wrong
  value on a nested node is caught. Optional containers are deliberately
  not descended into — WA models mutually exclusive alternatives as
  several optional same-node mixins (a newsletter `<meta>` pins
  `interaction_type` to both "question" and "question_reshare"), so
  descending into all of them would build a self-contradicting node.
- `checked > 0` stayed green on a layer that had degraded to tag-only;
  the echo/pin coverage is floored too.
- `handler_without_a_const_keyed_switch_has_no_actions` was vacuous: the
  fixture dispatched only `w:gp2`, so the loop body never ran. It now
  dispatches a second, action-less type, and asserts the fixture does so.

Rejected: scoping action discovery to the notification's `handler_function`.
The switch is not in the handler — `handleGroupNotification` builds a
`WADeprecatedWapParser` around a sibling local and the child-tag switch
lives there, so scoping to the handler body finds nothing and loses all 45
`w:gp2` arms. The underlying risk (a module hosting two handlers with two
switches) is real but latent; a module serving more than one notification
type is now skipped instead, so the failure is a visible omission rather
than a union attributed to the wrong type.

Determinism holds, the schemas validate, and the reference Rust codegen is
still byte-identical.
@jlucaso1

Copy link
Copy Markdown
Contributor Author

Review round 2 — CodeRabbit + Codex

17 inline findings: 16 applied in 08c01ed, 1 rejected with evidence (replied individually on every thread). Four changed the committed IR, and each was verified against the bundle before acting.

Wrong data that was actually shipping

Was Now
iq_constraint_counts double-counted the primary success variant fieldLiterals 2174, fieldEnumRefs 277 1782 / 146
Error vocabulary read only from UnionVariant fields 29 error variants had none all 29 populated
Vocabulary computed for every variant kind 3 PreKeys success variants carried a bogus error set gated on Error
Optional echo consumed as a presence flag pin lost, and not even counted 5 recovered, referenceConstraints 579 → 584

The counter drop is de-duplication, not loss — response.fields is byte-identical to the primary success variant's fields in 97 of 143 stanzas, and counting the old IR with the fixed logic gives exactly 1782/146. One --allow-shrink was needed to move past the inflated floor. The floor guard doing its job on its own inflated baseline is, in a backhanded way, the feature working.

The c_dhash finding (Codex, P1) deserves singling out: I had reported the optionalLiteral drop counter going 10 → 0 as a win in the previous round. It was partly a regression — carrying reference_path on a binding that is never consumed as a field turned a counted drop into a silent one. Correcting the record here as well as in the code.

Latent correctness

optionalLiteral mis-typing a non-string pin; a request-derived field losing its path; unbounded find_accessor recursion on mutually referential minified bindings; a merged action branch leaving an absent field required; a hoisted content receiver yielding an empty wire name; returns nested in if/else being skipped; a reused minified identifier resolving a const table to the wrong object. None changes today's output; all are guarded now.

The round-trip guard was too shallow

Codex was right that it modelled only the root, so it skipped every sourcePath and nested-child pin — it would have accepted the malformed promote response its own header describes. It now models the stanza as a path-addressed tree: 175 nested pins exercised where there were zero, with a regression test that participant/@type = "200" is caught, plus the echo/pin coverage floor CodeRabbit asked for (119 of 141 shapes constrained).

Building that immediately surfaced something the flat walk could not see: WA models mutually exclusive alternatives as several optional same-node mixins, not as a union — a newsletter <status><meta> carries one mixin pinning interaction_type="question" and another pinning "question_reshare" on the same attribute. Both pins are correct; only one applies. Optional containers are therefore not descended into, and that is documented where it matters.

Rejected

Scoping action discovery to handler_function (Codex, P2). The switch is not in the handler: handleGroupNotification builds a WADeprecatedWapParser around a sibling local, and the child-tag switch lives there — scoping to the handler body finds nothing and loses all 45 w:gp2 arms. The hazard is real but latent (no module serves more than one notification type today), so a shared module is now skipped instead: a visible omission rather than a union attributed to the wrong type.

Validation

fmt --check; clippy --all-targets and --all-features --all-targets, both -D warnings; cargo test --workspace (35 suites, +3 new tests); wa-ir --features schema; wa-ir on wasm32; validate-schemas.py 12/12; determinism update --check27 committed artifact(s) up to date; codegen A/B still byte-identical at 23 059 lines.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 08c01ed314

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-scan/src/response_smax.rs
Comment thread crates/wa-scan/src/response_smax.rs
Comment thread crates/wa-scan/src/response_smax.rs Outdated
Ten further findings from two Codex passes (one landed mid-work on 5500115,
one on 08c01ed). Nine applied, one already fixed, one adjudicated against a
finding from the same reviewer.

The flattened error vocabulary was ambiguous — the same failure class the
domain exists to prevent. `errorCodes: [400, 501]` beside `errorTexts:
["bad-request", "feature-not-implemented"]` admits four combinations when
the parser accepts two, so an emitter picking one value from each list can
produce `400 feature-not-implemented`, which matches no branch and is
unparseable. 117 variants were ambiguous this way. `ResponseVariant` now
carries `errorArms: [{name, code, text, codeMin, codeMax}]` — the accepted
shapes, paired — and the flat lists are derived from it as a view, so the
two can never disagree.

Data that was being lost:

- A required echo on a DESCENDED node vanished entirely. `literal` on a node
  reached through `flattenedChildWithTag` records no root assertion (the
  assertion vocabulary is root-relative) and the initializer discarded the
  reference too, so a nested attribute that must mirror the request reached
  the IR with no constraint at all.
- `locked`'s `threshold` was missing because the minifier hides the accessor
  in the ternary's TEST — `(n = child.maybeAttrString("threshold")) != null
  ? n : void 0` — and following the consequent alone finds a bare `n` that
  no declaration binds.
- `unlink` reported a single `delete_parent_group_unlink`. Its helper
  branches into four different actions, and the recursive return collector
  added last round exposed them only to have them folded into one — keeping
  whichever `actionType` resolved first. A wholesale delegation now expands
  the helper's branches into sibling arms and runs them through the same
  merge-by-`actionType` grouping the switch arms use. 55 -> 58 arms.

Latent:

- A helper name bound twice now resolves to nothing rather than
  last-one-wins, so a nested function reusing a minified identifier cannot
  make an arm inline an unrelated function. Same rule the constant tables
  already use.
- A switch fall-through label (`case A: case B: return {…}`) inherits the
  next executed body instead of being recorded as an empty action. Not
  present in today's bundle.

Diagnostics:

- The enum drop counter double-counted: the same parser source is analyzed
  twice (once for fields, once for assertions) and a module is reachable
  from several RPCs, so per-invocation counting reported one lost enum many
  times. CodeRabbit asked for per-occurrence and Codex for once-per-loss;
  both are right about the failure and the unit is the disagreement. Drops
  are now keyed by the constraint lost, so the number means "distinct
  constraints" — back to 10, and stable across analysis passes.
- The notif floor counted ARMS, which cannot regress while the switch is
  still recognized: every case yields one arm even if its whole shape
  vanished. `actionShapes` (resolved action types + fields + constants +
  child fields, 181) is floored alongside it, as is `errorArms` (639).

Adjudicated: carrying an optional echo on a `Bool` presence flag, which the
previous round added to recover the blocklist `c_dhash` rule, was then
flagged for conflating an always-present boolean with an optional string
attribute. Both halves are right — the rule must be recorded, and
`required: true` on the flag must not be read as "the attribute is
required". The rule is now explicit rather than inferred: on a `Bool` field
the pin constrains the underlying attribute *when present*, never the
boolean, documented on both `literalValue` and `referencePath`. The
round-trip guard was demonstrating the harm (it demanded an optional
`c_dhash` and compared a request string with a boolean) and now honours the
rule, with a regression test.

Already fixed before the comment landed: gating the whole error vocabulary
on `kind == Error` (that pass reviewed 5500115; 08c01ed had it).

No `--allow-shrink` needed. Determinism holds, schemas validate, and the
reference codegen is still byte-identical.
@jlucaso1

Copy link
Copy Markdown
Contributor Author

Review round 3 — 10 further Codex findings

Two more Codex passes (one landed on 5500115 while I was working, one on 08c01ed). 9 applied in 28b8b14, 1 already fixed, and one adjudicated against another finding from the same reviewer. Replied on every thread.

The flattened error vocabulary was reproducing the bug it exists to prevent

errorCodes: [400, 501] beside errorTexts: ["bad-request", "feature-not-implemented"] admits four combinations where the parser accepts two. An emitter picking one value from each list sends 400 feature-not-implemented — matches no branch, unparseable. 117 variants were ambiguous this way.

ResponseVariant now carries the pairing, and the flat lists are derived from it so the two cannot drift:

"errorArms": [
  { "name": "IQErrorBadRequest",           "code": 400, "text": "bad-request" },
  { "name": "IQErrorFeatureNotImplemented","code": 501, "text": "feature-not-implemented" },
  { "name": "IQErrorFallbackServer",       "codeMin": 500, "codeMax": 599 }  // any text in range
],
"errorCodes": [400, 501],                    // the union — for "does X accept N?"
"errorTexts": ["bad-request", "feature-not-implemented"]

639 arms, floored.

Three real losses

Was Now
Required echo on a node reached via flattenedChildWithTag no assertion and no referencePath — no constraint at all carried on the sourced field
locked's threshold field missing — the accessor hides in the ternary's test, (n = …maybeAttrString("threshold")) != null ? n : void 0 recovered, optional
unlink one delete_parent_group_unlink — its helper branches into four different actions, conflated into one 4 arms; total 55 → 58

The unlink one is a regression my own previous fix introduced: the recursive return collector exposed the helper's branches only for fold_shape to fold them together. A wholesale delegation now expands into sibling arms and reuses the switch-arm grouping.

Two reviewers disagreeing — twice

The enum drop counter. CodeRabbit asked for per-occurrence; Codex then showed per-invocation inflates, because the same parser source is analyzed twice (fields, then assertions) and a module is reachable from several RPCs — the 65 was noise. Both identified a real defect and disagreed on the unit. Drops are now keyed by the constraint lost, so the number means "distinct constraints", stated in the doc and stable across passes: back to 10.

The pin on a presence flag. Round 2 added referencePath to hasListCDhash to recover the blocklist c_dhash rule (Codex P1). Round 3 flagged that same field for conflating an always-present boolean with an optional string attribute (Codex P2). Both are right: the rule must be recorded, and required: true on the flag must not read as "the attribute is required". Resolved by making it explicit rather than inferred — on a Bool field the pin constrains the underlying attribute when present, never the boolean, documented on both literalValue and referencePath. The concrete harm was real and I had shipped it: the round-trip emitter demanded the optional c_dhash and compared a request string against a boolean, passing only because it was self-consistently wrong.

Also

A helper name bound twice now resolves to nothing rather than last-one-wins (the rule the constant tables already use); a switch fall-through label inherits the next executed body (not present in today's bundle); and diagnostics.notif.actionShapes (181) is floored alongside actions, since the arm count cannot regress while the switch is still recognized — every case yields one arm even if its whole shape vanished.

Validation

No --allow-shrink this time. fmt --check; clippy and clippy --all-features, both -D warnings; 35 suites (+4 new tests, 40 total added by the PR); schema self-tests; wa-ir on wasm32; validate-schemas.py 12/12; determinism update --check27 committed artifact(s) up to date; codegen A/B still byte-identical. Round-trip: 141 shapes, 119 constrained, 180 pinned fields exercised.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 28b8b14f41

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-scan/src/response_index.rs
Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-notif/src/actions.rs
…nd 3 left

Four more Codex findings on 28b8b14. All applied; two were live, two latent.

A nested payload union erased its parent's pins. An error alternative can
both pin a code/text and carry a further union of reasons that pin nothing:
`CreateOrAddGroupRateLimitError` pins 429/rate-overlimit and nests
`AddParticipantTimeRateLimit` / `AddParticipantCountRateLimit`. The nested
leaves replaced the outer arm, so `errorArms` — documented as the
authoritative form — published four entries with no usable pairing while
the shape the server actually sends was absent. Nested arms now inherit the
outer pins wherever they have none of their own, and the specific inner name
still wins. 4 empty arms -> 0.

Accessor aliases were typed as strings. `find_accessor` returns the raw
method name and `wap::method_field_type` did not know `attrPhoneUserJid`,
`maybeAttrPhoneUserJid`, `attrPhoneDeviceJid`, `attrJidEnum`,
`attrDomainJid`, `attrLidJid`, `attrIntRange`, `attrStringEnum` or
`attrEnumOrNullIfUnknown`, while `is_wire_accessor` deliberately accepts
every `attr*` — so those fields were kept and silently mistyped. Live case:
the group-notification `phone_number`, a PN user JID reported as `string`,
which is precisely the conflation that function's own doc comment calls
protocol-safety-critical. Fixed in the shared classifier rather than locally,
since it is wrong everywhere; blast radius is nil — none of those spellings
appears as a `method` in any committed domain today.

Two latent gaps the previous round opened or left:

- Returns are now collected through `if`/`else`, but the scope was still
  built from direct declarations only, so `if (c) { var id =
  child.attrString("id"); return {id} }` yielded a return whose `id`
  resolved to nothing. `var` is function-scoped, so bindings are now
  collected through the same control flow.
- An expression-bodied arrow callback (`p => wid(p.attrUserJid("jid"))`)
  has no `return` — oxc stores the body as a lone expression — so a
  repeated child was catalogued with an empty field list despite the
  comment promising that shape. Handled for callbacks and for inlined
  helpers alike.

Neither changes today's output (WA's minifier emits `function(){}`, and no
arm declares inside a nested block), so both are covered by fixture cases
instead.

Determinism holds, schemas validate, codegen still byte-identical, and no
--allow-shrink was needed.
@jlucaso1

Copy link
Copy Markdown
Contributor Author

Review round 4 — 4 further Codex findings

All applied in bd585b6. Two were live; two were latent gaps the previous round opened or left.

A nested payload union was erasing its parent's pins

An error alternative can both pin a code/text and carry a further union of reasons that pin nothing. CreateOrAddGroupRateLimitError pins 429/rate-overlimit and nests two reasons — and the nested leaves replaced the outer arm, so errorArms (documented as the authoritative form) published entries with no usable pairing while the shape the server actually sends was absent:

// was                                    // now
{ "name": "AddParticipantTimeRateLimit" }  { "name": "AddParticipantTimeRateLimit",  "code": 429, "text": "rate-overlimit" }
{ "name": "AddParticipantCountRateLimit" } { "name": "AddParticipantCountRateLimit", "code": 429, "text": "rate-overlimit" }

Empty arms 4 → 0. This one was a defect I introduced in the previous commit — the arms model is new, and this was its first real stress.

Accessor aliases were silently mistyped

find_accessor returns the raw method name and is_wire_accessor deliberately accepts every attr*, but wap::method_field_type knew none of attrPhoneUserJid, maybeAttrPhoneUserJid, attrPhoneDeviceJid, attrJidEnum, attrDomainJid, attrLidJid, attrIntRange, attrStringEnum, attrEnumOrNullIfUnknown — so those fields were kept and typed string. Live case: the group-notification phone_number, a PN user JID reported as a string, which is precisely the conflation that function's own doc comment calls protocol-safety-critical.

Fixed in the shared classifier rather than locally, because it is wrong everywhere. Verified the blast radius first: none of those spellings appears as a method value in any committed domain, so the only output change is phone_numberuser_jid. (This was the follow-up I had deferred out of the original PR; Codex found it independently, and measuring showed deferring was unnecessary.)

Two latent gaps

Returns were being collected through if/else while the scope was still built from direct declarations only — fixed at the semantically right level, since var is function-scoped, so bindings are now collected through the same control flow. And an expression-bodied arrow callback has no return (oxc stores the body as a lone expression), so a repeated child was catalogued with an empty field list despite the comment promising that shape; fn_result_shapes now uses get_expression(), and both helper-inlining paths were routed through it — they had the same hole.

Neither changes today's output, so both are covered by fixture cases rather than asserted against the bundle.

Validation

fmt --check; clippy and --all-features, both -D warnings; 35 suites, +5 tests this round (45 added by the PR); schema self-tests; wasm32; validate-schemas.py 12/12; determinism update --check27 committed artifact(s) up to date; codegen A/B byte-identical; round-trip 141 shapes / 119 constrained / 180 pins. No --allow-shrink.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
crates/wa-ir/src/wap.rs (1)

118-143: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

maybeAttrPhoneDeviceJid is unhandled, and the new spellings aren't covered by the tests or the is_optional_method/is_attr_method predicates.

Line 121 handles both attrPhoneUserJid and its maybe* alias, but Line 122 handles only attrPhoneDeviceJid — a maybeAttrPhoneDeviceJid read falls through to the _ => String default, which is exactly the JID/string conflation this block was added to fix. Same question applies to the other raw spellings added here (attrPhoneChatJid, attrFromPhoneJid, attrEnumOrNullIfUnknown, …): if is_optional_method/is_attr_method are literal-set based rather than prefix based, these will be classified as required/non-attr. None of the new arms are asserted in optional_and_field_types or notification_accessors_classified, so a dropped arm is invisible.

#!/bin/bash
# Which of these accessor spellings actually occur, and how the predicates classify them.
fd -e rs . crates/wa-ir/src --exec rg -n 'fn is_optional_method|fn is_attr_method' -A 20
rg -n 'maybeAttrPhoneDeviceJid|attrPhoneDeviceJid|attrFromPhoneJid|attrPhoneChatJid|attrEnumOrNullIfUnknown' -g '!target' .
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/wa-ir/src/wap.rs` around lines 118 - 143, Update the accessor
classification around the parsed-field type match to include
maybeAttrPhoneDeviceJid as ParsedFieldType::DeviceJid, and ensure every newly
supported spelling is recognized consistently by is_optional_method and
is_attr_method. Extend optional_and_field_types and
notification_accessors_classified with assertions covering the new JID and enum
accessors so classification regressions are detected.
README.md (1)

29-29: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Mention errorArms — the authoritative paired form — in the error-vocabulary bullet.

The bullet lists only the flattened fields, but ResponseVariant::error_arms is documented in the IR as the authoritative shape and an emitter must pick a (code, text) pair from it rather than one value from each flat list. Omitting it here points readers at exactly the combination hazard the paragraph warns about.

📝 Suggested wording
-- **The per-RPC error vocabulary** (`errorClass`, `errorCodes`, `errorTexts`, `errorCodeMin`/`Max` on a response variant) — a **closed** set, and it differs per RPC:
+- **The per-RPC error vocabulary** (`errorArms`, plus the derived `errorClass`, `errorCodes`, `errorTexts`, `errorCodeMin`/`Max` on a response variant) — a **closed** set, and it differs per RPC. `errorArms` is the authoritative form: it says *which* code goes with *which* text, so an emitter picks a pair from it instead of combining one value from `errorCodes` with another from `errorTexts`:
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@README.md` at line 29, Update the per-RPC error vocabulary bullet in
README.md to mention ResponseVariant::error_arms alongside the flattened error
fields, identifying it as the authoritative paired representation that preserves
valid (code, text) combinations and must be used by emitters.
♻️ Duplicate comments (1)
crates/wa-scan/src/response_smax.rs (1)

137-146: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

The "count per OCCURRENCE" comment contradicts the keyed-set mechanism, and the cache-hit branch is a no-op.

drops is a BTreeMap<reason, BTreeSet<key>> and drop_counts() reports keys.len(), so drop_note_keyed(ENUM_DROP, "{module}.{name}") on a cache hit re-inserts a key the miss path already inserted — N fields validating against the same unresolvable enum still report 1, not N. Either the comment here or the struct doc at Lines 83-93 ("The unit is distinct constraints") is wrong; the code implements the latter. Suggest dropping the misleading comment and the dead branch, or making the key occurrence-unique if per-occurrence counting is actually intended.

♻️ Align the comment with the keyed semantics
-        // Count per OCCURRENCE, not per distinct enum: `dropsByReason` measures how much
-        // constraint data was lost, and every other reason here is per-occurrence. A
-        // memoized miss must therefore still be counted, so N fields validating against
-        // the same unresolvable enum report N losses rather than one.
+        // A memoized miss is still recorded, but the key is the enum identity, so the
+        // reported unit stays "distinct unresolvable enums" (see `Resolver::drops`).
         if let Some(hit) = self.enum_cache.borrow().get(&key) {
             if hit.is_none() {
                 self.drop_note_keyed(ENUM_DROP, format!("{module}.{name}"));
             }
             return hit.clone();
         }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/wa-scan/src/response_smax.rs` around lines 137 - 146, Update the
enum-cache hit handling in the surrounding response validation method to match
the keyed distinct-constraint semantics: remove the misleading “count per
OCCURRENCE” explanation and the cache-hit branch that calls drop_note_keyed,
while preserving the cached result return behavior. Keep drop_note_keyed on the
actual miss path unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/wa-notif/src/actions.rs`:
- Around line 464-493: Update collect_bindings in crates/wa-notif/src/actions.rs
at lines 464-493 so a repeated name with a different initializer is removed or
marked ambiguous instead of retaining the first binding, matching
ConstCollector.locals and extract_actions by_name behavior; preserve first-wins
only for identical bindings. Extend the fixture in crates/wa-notif/src/tests.rs
at lines 817-835 with same-name var declarations across branches and assert each
return resolves to its own wire name.

---

Outside diff comments:
In `@crates/wa-ir/src/wap.rs`:
- Around line 118-143: Update the accessor classification around the
parsed-field type match to include maybeAttrPhoneDeviceJid as
ParsedFieldType::DeviceJid, and ensure every newly supported spelling is
recognized consistently by is_optional_method and is_attr_method. Extend
optional_and_field_types and notification_accessors_classified with assertions
covering the new JID and enum accessors so classification regressions are
detected.

In `@README.md`:
- Line 29: Update the per-RPC error vocabulary bullet in README.md to mention
ResponseVariant::error_arms alongside the flattened error fields, identifying it
as the authoritative paired representation that preserves valid (code, text)
combinations and must be used by emitters.

---

Duplicate comments:
In `@crates/wa-scan/src/response_smax.rs`:
- Around line 137-146: Update the enum-cache hit handling in the surrounding
response validation method to match the keyed distinct-constraint semantics:
remove the misleading “count per OCCURRENCE” explanation and the cache-hit
branch that calls drop_note_keyed, while preserving the cached result return
behavior. Keep drop_note_keyed on the actual miss path unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4e136593-0fe7-4c22-a9b1-869fa022ad61

📥 Commits

Reviewing files that changed from the base of the PR and between ced8322 and bd585b6.

⛔ Files ignored due to path filters (8)
  • generated/iq/index.json is excluded by !**/generated/**
  • generated/manifest.json is excluded by !**/generated/**
  • generated/notif/index.json is excluded by !**/generated/**
  • generated/schema/incoming.schema.json is excluded by !**/generated/**
  • generated/schema/iq.schema.json is excluded by !**/generated/**
  • generated/schema/notif.schema.json is excluded by !**/generated/**
  • generated/schema/srvreq.schema.json is excluded by !**/generated/**
  • generated/schema/stanza.schema.json is excluded by !**/generated/**
📒 Files selected for processing (11)
  • README.md
  • crates/wa-fetch/src/cache.rs
  • crates/wa-ir/src/iq.rs
  • crates/wa-ir/src/wap.rs
  • crates/wa-notif/src/actions.rs
  • crates/wa-notif/src/lib.rs
  • crates/wa-notif/src/tests.rs
  • crates/wa-scan/src/response_index.rs
  • crates/wa-scan/src/response_smax.rs
  • crates/wa-scan/tests/iq_roundtrip.rs
  • crates/whatspec/src/main.rs

Comment thread crates/wa-notif/src/actions.rs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bd585b6406

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-ir/src/lib.rs Outdated
Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-scan/src/lib.rs Outdated
Comment thread crates/wa-notif/src/actions.rs Outdated
Six findings on bd585b6 — one from CodeRabbit, five from Codex, two of them
the same defect from both. All applied.

schemaVersion 1.1.0 -> 2.0.0. Nearly everything this branch adds is
optional and a 1.x consumer ignores it, but `AssertionKind` gained a
`reference` variant, which widens the value space of an EXISTING field. A
consumer with a closed enum — generated from the 1.0 JSON Schema, or the
Rust enum itself — rejects the document rather than ignoring an unknown
key, and that is not hypothetical: validating the current `iq/index.json`
against the committed 1.0 schema fails with 579 errors, all `'reference' is
not valid under any of the given schemas`. Shipping that as a minor bump
would tell consumers the opposite of the truth. The migration is one
variant wide, and the reasoning is recorded on the constant.

A flattened branch scope produced WRONG field names, not missing ones.
Making return collection recursive last round left the scope flat and
first-wins, so mutually exclusive branches rebinding the same minified name
made one branch's return read the other branch's attribute. Both reviewers
flagged it; CodeRabbit proposed refusing the ambiguity, Codex proposed
per-return scopes. Per-return is strictly better — it keeps both branches
correct instead of dropping both — so shapes now carry the scope in force
where they are returned. The one caller with no return path to attribute a
binding to (the object-property walker) does refuse ambiguity, which is the
"missing, not wrong" rule the rest of the module already follows.

Also:

- `guard && value` carries the value on the RIGHT; taking the left yielded
  `hasAttr`, deliberately not a wire accessor, so the field vanished.
  `a || b` keeps the left, where the value really is.
- Notification action fields can carry `enumRef`, resolved through the same
  `wa_enums` path the request and response sides use, including when the
  enum argument is hoisted into a local. Currently latent in the bundle —
  the one live enum field (`create.reason`) validates against an ANONYMOUS
  inline table, whose identifier is whatever the minifier picked, so naming
  it would churn the IR every rollout and invent an identity the bundle
  does not have. Left unresolved and documented as such rather than faked.
- Drops found while resolving a fallback response were being discarded: the
  fallback builds its own resolver during the scan, and the snapshot was
  taken before that from the build-pass resolver only. The collector is now
  shared and snapshotted after the scan, so the diagnostic guarantee holds
  for exactly the unusual shapes the fallback exists to serve.

Determinism holds, schemas validate, codegen still byte-identical, no
--allow-shrink.
@jlucaso1

Copy link
Copy Markdown
Contributor Author

Review round 5 — 6 findings, and a contract correction

One from CodeRabbit, five from Codex (two of them the same defect from both). All applied in 1bf091c.

The schema version was wrong, and I checked instead of arguing

Codex pushed back on 1.1.0. Verified against the committed 1.0 schema: the current iq/index.json fails validation with 579 errors, every one 'reference' is not valid under any of the given schemas. Nearly everything this branch adds is optional and a 1.x consumer ignores it — but AssertionKind gained a variant, which widens the value space of an existing field, and a closed-enum consumer rejects the document rather than ignoring a key. Shipping generated/schema/ and then calling that a minor bump tells consumers the opposite of the truth.

schemaVersion → 2.0.0. Not re-encoded to be skippable, because that would be worse: kind: "attr" with a referencePath and no value reads as "must equal null" to an old deserializer — silently wrong beats loudly incompatible. The migration is one variant wide and is recorded on the constant and in the README.

A flattened branch scope produced wrong names, not missing ones

Making return collection recursive last round left the scope flat and first-wins, so mutually exclusive branches rebinding the same minified name made one branch's return read the other branch's attribute. Both reviewers caught it, from opposite directions: CodeRabbit proposed refusing the ambiguity, Codex proposed per-return scopes.

Went with per-return — strictly better, since it keeps both branches correct rather than dropping both. CodeRabbit's rule still governs the one caller with no return path to attribute a binding to (the object-property walker), which now refuses ambiguity like the constant tables and helper names do.

Worth noting this was my error compounding: that commit's message argued first-wins was fine "because var hoists". Hoisting moves the declaration; the branch that executes still assigns its own value.

Also

guard && value carries the value on the right — taking the left yielded hasAttr, deliberately not a wire accessor, so the field vanished. And drops found while resolving a fallback response were being discarded, because the fallback builds its own resolver during the scan while the snapshot was taken beforehand — defeating the diagnostic guarantee for exactly the unusual shapes the fallback exists to serve. Collector is now shared and read after the scan.

One left deliberately open

NotifActionField can now carry enumRef (same wa_enums path as the other two domains, including a hoisted enum argument). But Codex's live example, create.reason, is not a named enum: it validates against an anonymous object literal declared inline in the handler. Its keys really are the legal wire values, so the information exists — but the table's identifier is whatever the minifier picked this build, so publishing it would invent an identity the bundle lacks and churn the IR every rollout.

Left unresolved and documented on the field rather than faked. Recovering anonymous value sets is a genuine gap, but it is a modelling question — what identifies an unnamed enum across builds? — and deserves a deliberate decision, not one taken at the end of a review round.

Validation

fmt --check; clippy and --all-features, both -D warnings; 35 suites, +3 tests (48 added by the PR); schema self-tests; wasm32; validate-schemas.py 12/12; determinism update --check27 committed artifact(s) up to date; codegen A/B byte-identical; round-trip 141 / 119 constrained / 180 pins. No --allow-shrink.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1bf091cdb9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-scan/src/response_smax.rs Outdated
Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-scan/src/response_index.rs Outdated
…a required read

Five findings on 1bf091c. Three applied; two rejected on evidence.

Applied:

- A helper used in VALUE position (an `extends` operand, an object-property
  value) had all its result branches accumulated into one definition, so a
  helper returning `{x}` in one branch and `{y}` in another made the
  enclosing action require both and reject either legal payload. Branches
  are now folded separately and merged with `merge_action`, the same
  optionality rule the switch arms use. The wholesale-delegation path got
  this last round; this is its value-position twin.
- `a || b` / `a ?? b` were classified as presence guards, so a required
  read with a default (`child.attrString("code") || "none"`) came out
  optional — `attrString` still rejects an absent attribute; the right
  operand only defaults a value the parser already demanded. Only a ternary
  and `guard && value` gate a read now.
- An unresolvable `literal` on a node reached through `flattenedChildWithTag`
  reported nothing: the drop note sat inside the same `source_path.is_none()`
  guard as the root assertion, so the field carried no pin while
  `dropsByReason` simultaneously claimed nothing was lost. The assertion is
  root-relative and stays gated; the drop is not and no longer is.

Rejected, with the checks:

- "Include validation-only pins in error arms" — the premise does not hold
  in this bundle. Swept every error variant for a `code`/`text` pinned in a
  `UnionVariant`'s assertions but missing from its fields: zero. The cited
  shape is `IQErrorReportTokenValidationFail`, whose arm carries `code: 548`
  and no `text` — and that is the fact, not a gap: the mixin reads
  `attrString(e, "text")` with no `literal`, so it accepts ANY text with
  that code. Sourcing arms from assertions would be dead code today; the
  actionable part was that a code-only arm looked like a defect, so
  `ErrorArm::text` now documents both ways absence arises (a fallback arm
  pinning neither value, and a code-only arm reading the text freely).
- "Bind helper parameters to call arguments" — latent, and the fix is
  structurally awkward. Every wholesale delegation in the bundle passes
  NODES (`T(chat, child, author)`, `I(child)`), which need no binding since
  accessors are read structurally off whatever receiver they have; a helper
  taking a wire VALUE appears nowhere. Seeding it would also have to pair
  parameter names from the helper's reparse with argument expressions from
  the caller's — two different allocators — for no present gain.

Determinism holds, schemas validate, codegen byte-identical, no
--allow-shrink.
@jlucaso1

Copy link
Copy Markdown
Contributor Author

Review round 6 — 5 findings: 3 applied, 2 rejected on evidence

CodeRabbit approved 1bf091c. Codex raised five more; f71f94a applies three and rejects two after checking.

Applied

  • A value-position helper's branches were accumulated, not merged. A helper used as an extends operand returning {x} in one branch and {y} in another made the enclosing action require both and reject either legal payload. This is the twin of the wholesale-delegation fix from last round — I applied that one on only one side. Branches now fold separately and merge with the same optionality rule.
  • a || b was being read as a presence guard, so a required read with a default (attrString("code") || "none") came out optional. attrString still rejects an absent attribute; the right operand only defaults a value the parser already demanded. Another drift of my own: I changed strip_guard to take the right operand of && last round without revisiting is_guarded.
  • An unresolvable literal on a descended node reported nothing. The drop note sat inside the same source_path.is_none() guard as the root assertion, so the field carried no pin and dropsByReason claimed nothing was lost — precisely the confusion the counter exists to prevent.

Rejected

"Include validation-only pins in error arms." The premise does not hold here. Swept every error variant for a code/text pinned in a UnionVariant's assertions but missing from its fields: zero. The cited case, IQErrorReportTokenValidationFail, is:

assertTag(e,"error"); attrString(e,"text"); literal(attrInt, e, "code", 548)

text is read with a plain attrString — no literal — so the arm accepting any text with code 548 is the fact, not a gap. The actionable part was that a code-only arm looks like a defect, so ErrorArm::text now documents both ways absence arises.

"Bind helper parameters to call arguments." Latent: every wholesale delegation in the bundle passes nodes (T(chat, child, author), I(child)), which need no binding since accessors are read structurally off whatever receiver they have. A helper taking a wire value appears nowhere, and where one is passed to a local it is a wrapper find_accessor already descends into. Seeding it would also mean pairing parameter names from the helper's reparse with argument expressions from the caller's — two different allocators — for no present gain.

Where this stands

Six rounds, 42 findings. Worth being straight about the shape of them: four were regressions I introduced while fixing earlier findings, including two in this round. The genuinely-new-defect rate has not fallen, but the severity has — this round's applied three are all latent in the current bundle, none changes a byte of generated/, and the two rejections were the first where checking beat applying.

That is my read of the natural stopping point. The model (errorArms, per-return scopes, the constraint layer) is settled, the committed IR is unchanged by this round, and further passes look likely to keep finding hypotheticals in extractor code rather than defects in the contract. Happy to run more if you'd rather exhaust it.

Validation

fmt --check; clippy and --all-features, both -D warnings; 35 suites, +2 tests (50 added by the PR); schema self-tests; wasm32; validate-schemas.py 12/12; determinism update --check27 committed artifact(s) up to date; codegen A/B byte-identical; round-trip 141 / 119 / 180. No --allow-shrink, and generated/ is byte-identical to the previous round.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f71f94a175

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-scan/src/response_index.rs Outdated
Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-notif/src/actions.rs
…child branches

Three findings on f71f94a. All applied, and one of them caught a mistake I
made two rounds earlier.

`SetResponsePreKeySuccessVnameFailure` was classified a SUCCESS because
`variant_kind` reads only the tag's name tokens, and every token in that
name says success. It asserts `type="error"` and parses an `<error
code="207">` wrapping an inner `<error code=400..599>`: the prekeys landed,
the vname did not, and the server said so with an error stanza. Because the
error vocabulary is gated on the kind, the codes and texts it accepts were
being dropped.

The classification now takes the parser's root discriminator first and
falls back to the tag tokens, so the protocol decides rather than a naming
convention. That also corrects round 2 of this review: I stripped the
vocabulary from three "success" variants as bogus, and for two of them
(`FetchKeyBundlesResponseSuccess`, `FetchMissingPreKeysResponseSuccess`,
both asserting `type="result"` with per-item nested errors) that was right,
but for this one it was not. Exactly one variant reclassifies, and it goes
from an empty vocabulary to `[207]` / three texts / range 400–599.

Its envelope code needed one more thing: a two-level error has pins no ARM
can carry, since an arm describes one innermost alternative and the outer
`code="207"` belongs to none of them. The variant's own pins are now folded
into the flat vocabulary so the envelope is not simply lost, while
`errorArms` stays the per-alternative view.

Also:

- A `mapChildrenWithTag` callback returning different shapes from an `if`
  had every property in its body flattened into one child definition,
  marking a field read in only one branch as required. Callbacks are now
  read per return shape and merged — the third and last place that rule
  was missing.
- A constant table assigned to a non-export receiver (`cache.GROUP_ACTIONS
  = local` before the real export) could be taken as the export and, being
  first, kept — resolving case labels through an unrelated table. Only the
  module factory's own parameters count as receivers now, falling back to
  the old rule when the factory shape is unrecognised, so a narrower rule
  cannot turn an unfamiliar module into a silently empty one.

The mapped-child change initially cost 49 shape elements — the floor guard
caught it, which is what it is for. Two causes, both the minifier's doing:
the object is built into a local and returned by name, and the return is a
comma expression (`return sideEffect(), u`) whose value is its last
element. Both are handled, and coverage is back to 181 with no
`--allow-shrink`.

Round-trip coverage moves 141 -> 140 shapes: the reclassified variant is no
longer a success shape, so the guard no longer walks it.

Determinism holds, schemas validate, codegen byte-identical.
@jlucaso1

Copy link
Copy Markdown
Contributor Author

Review round 7 — 3 findings, all applied, one correcting round 2

The classification was reading names instead of the wire

SetResponsePreKeySuccessVnameFailure was filed as a success because variant_kind reads only the tag's name tokens — and every token in that name says success. It asserts type="error" and parses <error code="207"> wrapping <error code=400..599>: the prekeys landed, the vname did not, and the server said so with an error stanza. Since the vocabulary is gated on the kind, everything it accepts was being dropped.

Classification now takes the parser's root discriminator first, falling back to tag tokens only when no type is pinned. Exactly one variant moves:

before: kind=success, errorCodes=[], errorTexts=[]
after:  kind=error,   errorCodes=[207], errorTexts=[bad-request, internal-server-error, not-acceptable], 400–599

This corrects round 2 of this review. I stripped the vocabulary from three "success" variants as bogus back then. For two of them that was right — they assert type="result" and merely carry per-item nested errors. For this one it was wrong, and I generalised across all three without checking each. The IR shipped that error for five rounds.

It also exposed a model limit worth naming: a two-level error has a pin no arm can carry, since an arm describes one innermost alternative and the outer code="207" belongs to none of them. The variant's own pins now fold into the flat vocabulary so the envelope is not lost, while errorArms stays the per-alternative view.

The floor guard earned its keep

The mapped-child branch merge — the third and last place that rule was missing — initially cost 49 shape elements, and the guard refused the write. Two causes, both minifier idioms my narrower walk stopped seeing: the object is built into a local and returned by name, and the return is a comma expression (return c || C(chat, u, tag), u) whose value is its last element. Both handled; coverage back to 181, no --allow-shrink.

That is the second time this branch's own guard has caught my regression before it reached generated/. Worth stating plainly since I argued for those counters in the first place.

Also

A constant table on a non-export receiver (cache.GROUP_ACTIONS = local before the real export) could be taken as the export and kept. Only the factory's own parameters count now — with a deliberate fallback when the factory shape is unrecognised, since a narrower rule that misfires would turn an unfamiliar module into a silently empty one, which is worse than the hazard.

Where this leaves it

Seven rounds, 45 findings. This round is the first where a finding corrected a wrong committed value that one of my earlier fixes introduced and that survived several passes — which argues the opposite of what I said last round about stopping. I was wrong that the severity had settled.

errorArms 639 → 643, actionShapes 181 (unchanged), round-trip 141 → 140 shapes (the reclassified variant is no longer a success shape, so the guard no longer walks it).

Validation

fmt --check; clippy and --all-features, both -D warnings; 35 suites, +2 tests (52 added by the PR); schema self-tests; wasm32; validate-schemas.py 12/12; determinism update --check27 committed artifact(s) up to date; codegen A/B byte-identical.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
crates/wa-scan/src/response_index.rs (1)

139-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Compute assertions once.

resolver.assertions(&module, &func) is called at line 139 and again at line 146. It is memoized so the cost is a map lookup and a Vec clone, but binding it once makes the kind/variant relationship obvious.

♻️ Bind once
-            let kind = variant_kind(&tag, &resolver.assertions(&module, &func));
+            let assertions = resolver.assertions(&module, &func);
+            let kind = variant_kind(&tag, &assertions);
             if kind == ResponseVariantKind::Success && primary.is_empty() {
                 primary = fields.clone();
             }
-            // The variant's same-node discriminators (e.g. `type:"result"` / `type:"error"`),
-            // recovered separately since the JS keeps them as parser asserts, not fields.
-            // These let codegen guard each arm so the outcome union doesn't misclassify.
-            let assertions = resolver.assertions(&module, &func);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/wa-scan/src/response_index.rs` around lines 139 - 146, Bind the result
of resolver.assertions(&module, &func) once before the variant_kind call, then
pass that binding to variant_kind and reuse it for the later assertions value.
Remove the duplicate resolver.assertions call while preserving the existing kind
and discriminator handling.
README.md (1)

29-29: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Document errorArms, not just the flattened lists.

errorArms is the authoritative representation this PR adds; errorCodes/errorTexts are explicitly a derived view that cannot express pairing (per crates/wa-scan/src/response_index.rs, an emitter combining one value from each list can produce 400 rate-overlimit, which the parser rejects). As written, the README points consumers at only the lossy form.

📝 Mention the paired form first
-- **The per-RPC error vocabulary** (`errorClass`, `errorCodes`, `errorTexts`, `errorCodeMin`/`Max` on a response variant) — a **closed** set, and it differs per RPC: `BatchGetGroupInfo` accepts `400 bad-request` and `429 rate-overlimit` and **rejects `404 item-not-found`**, even though that mixin exists and other RPCs use it. `errorCodeMin`/`Max` cover the open-ended fallback arms (any text, code in 400–499 / 500–599).
+- **The per-RPC error vocabulary** (`errorArms` on a response variant, plus the derived `errorClass`, `errorCodes`, `errorTexts`, `errorCodeMin`/`Max`) — a **closed** set, and it differs per RPC: `BatchGetGroupInfo` accepts `400 bad-request` and `429 rate-overlimit` and **rejects `404 item-not-found`**, even though that mixin exists and other RPCs use it. Each `errorArms` entry keeps `code` and `text` **paired**, so `400 rate-overlimit` — a combination the flattened lists cannot rule out — is never presented as legal. `errorCodeMin`/`Max` cover the open-ended fallback arms (any text, code in 400–499 / 500–599).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@README.md` at line 29, Update the README’s per-RPC error vocabulary
documentation to present errorArms as the authoritative, paired representation
first, including that each arm preserves the valid error code/text combinations.
Describe errorCodes and errorTexts only as derived, lossy views, and retain the
existing errorCodeMin/errorCodeMax explanation for open-ended fallback arms.
crates/wa-scan/src/response_smax.rs (1)

156-167: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

The "count per OCCURRENCE" comment contradicts the set-keyed collector.

drop_note_keyed inserts into a BTreeSet keyed by {module}.{name}, and drop_counts() reports keys.len(), so re-noting the same enum on a cache hit cannot change the count — the drop on line 163-165 is a no-op after the first miss, and the struct doc at lines 83-93 states the opposite ("the unit is distinct constraints"). Either drop the cache-hit note and fix this comment, or key the note per occurrence (e.g. include a call-site discriminator) if per-occurrence was really intended.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/wa-scan/src/response_smax.rs` around lines 156 - 167, The
occurrence-counting logic in resolve_enum conflicts with drop_note_keyed’s
set-based distinct-key counting. Remove the cache-hit drop_note_keyed call and
revise its comment to describe distinct enum constraints, preserving the initial
miss recording and existing cache behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/wa-notif/src/actions.rs`:
- Around line 73-97: Memoize enum_ref lookups by (module, name), matching the
existing member cache pattern. Add or reuse an enum-specific cache on the
resolver, return the cached AttrEnumRef when available, and cache successful
resolutions from wa_enums::resolve_named_enum while preserving the current
empty/invalid result behavior.

In `@crates/wa-notif/src/tests.rs`:
- Around line 930-934: The enum field assertion conflicts with the documented
enumRef contract. Reconcile the behavior in wap::method_field_type and the
enum_ref documentation in notif.rs: either return ParsedFieldType::Enum for
maybeAttrEnum fields, or revise the documentation to state that enumRef
accompanies the decoded scalar type, preserving the chosen behavior in this
test.

---

Outside diff comments:
In `@crates/wa-scan/src/response_index.rs`:
- Around line 139-146: Bind the result of resolver.assertions(&module, &func)
once before the variant_kind call, then pass that binding to variant_kind and
reuse it for the later assertions value. Remove the duplicate
resolver.assertions call while preserving the existing kind and discriminator
handling.

In `@crates/wa-scan/src/response_smax.rs`:
- Around line 156-167: The occurrence-counting logic in resolve_enum conflicts
with drop_note_keyed’s set-based distinct-key counting. Remove the cache-hit
drop_note_keyed call and revise its comment to describe distinct enum
constraints, preserving the initial miss recording and existing cache behavior.

In `@README.md`:
- Line 29: Update the README’s per-RPC error vocabulary documentation to present
errorArms as the authoritative, paired representation first, including that each
arm preserves the valid error code/text combinations. Describe errorCodes and
errorTexts only as derived, lossy views, and retain the existing
errorCodeMin/errorCodeMax explanation for open-ended fallback arms.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f7abc2c5-bc3c-4d75-b20e-644755a55666

📥 Commits

Reviewing files that changed from the base of the PR and between bd585b6 and 3552d0d.

⛔ Files ignored due to path filters (19)
  • Cargo.lock is excluded by !**/*.lock
  • generated/abprops/index.json is excluded by !**/generated/**
  • generated/appstate/index.json is excluded by !**/generated/**
  • generated/enums/index.json is excluded by !**/generated/**
  • generated/incoming/index.json is excluded by !**/generated/**
  • generated/iq/index.json is excluded by !**/generated/**
  • generated/manifest.json is excluded by !**/generated/**
  • generated/mex/index.json is excluded by !**/generated/**
  • generated/notif/index.json is excluded by !**/generated/**
  • generated/schema/incoming.schema.json is excluded by !**/generated/**
  • generated/schema/iq.schema.json is excluded by !**/generated/**
  • generated/schema/notif.schema.json is excluded by !**/generated/**
  • generated/schema/srvreq.schema.json is excluded by !**/generated/**
  • generated/schema/stanza.schema.json is excluded by !**/generated/**
  • generated/srvreq/index.json is excluded by !**/generated/**
  • generated/stanza/index.json is excluded by !**/generated/**
  • generated/tokens/index.json is excluded by !**/generated/**
  • generated/wam/index.json is excluded by !**/generated/**
  • generated/wasm/index.json is excluded by !**/generated/**
📒 Files selected for processing (10)
  • README.md
  • crates/wa-ir/src/iq.rs
  • crates/wa-ir/src/lib.rs
  • crates/wa-ir/src/notif.rs
  • crates/wa-notif/Cargo.toml
  • crates/wa-notif/src/actions.rs
  • crates/wa-notif/src/tests.rs
  • crates/wa-scan/src/lib.rs
  • crates/wa-scan/src/response_index.rs
  • crates/wa-scan/src/response_smax.rs

Comment thread crates/wa-notif/src/actions.rs
Comment thread crates/wa-notif/src/tests.rs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3552d0d3d4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-scan/src/response_index.rs Outdated
Comment thread crates/wa-notif/src/actions.rs
Comment thread crates/wa-scan/tests/iq_roundtrip.rs
…urce fields

Review round 19 — five findings, four applied.

The finalizer branch added last commit decided the override by asking whether the
finalizer had yielded any shape. That is wrong at both ends: `finally { if (c) return B }`
leaves A legal when `c` is false, and `finally { throw e }` yields nothing yet leaves the
vector empty, so A was published although it never escapes. Only a finalizer that leaves
on every path replaces the earlier shapes.

`find_accessor_at`'s wrapper descent took whichever argument read the wire first, so
`combine(c.attrString("a"), c.attrString("b"))` published `a` as the single source of a
value that depends on both. A `NotifActionField` names one `wireName` with one
requiredness, so two distinct reads are refused — the rule a key bound to two different
reads across branches already follows. The same read twice is not ambiguous and still
resolves.

`w:gp2`'s `create.reason` validates against a module-local table, which is not a nameable
`o("Mod").ENUM` reference, so it shipped as `"type": "enum"` with no values and no signal
anywhere. The action path has no linker to drain a pending marker, so it reports its own
losses: `diagnostics.notif.dropsByReason` now carries 1.

The smax enum-drop occurrence key was `site:attr`, which collapsed one parser reading the
same attribute off two descended nodes into a single loss. It includes the source path
now (no change in today's count — preventive).

Not applied: enforcing `literalValue` / `byteLength` / `byteMin`-`byteMax` in the
generated parsers. Real gap, but not a byte-content one — the reference codegen decodes
and does not validate ANY constraint: 2178 `literalValue`, 169 `intMin`, 88 `byteLength`,
281 `enumRef` all go unchecked today. Enforcing only the newest kind would be arbitrary;
the whole enforcement layer is its own change. Answered on the thread.

537 tests, 12/12 schemas, determinism check clean.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

_ => (None, None),

P2 Badge Record unresolvable byte-range bounds

When an smax contentBytesRange uses a constant identifier, computed expression, or otherwise non-literal bound, this returns the same (None, None) as an unconstrained content read. The caller consequently publishes a plain bytes field without byteMin/byteMax and without any drop diagnostic, so consumers cannot distinguish a missing constraint from no constraint; record an unresolved-range drop for this branch.



P2 Badge Report dropped request enum links

When a structurally recognized request enum cannot be resolved—for example because the exported table is runtime-composed, missing, non-string-valued, or the recovered guard is not a member—this silently clears enum_ref. Unlike response enum failures, EnumResolver returns no drop identity and the IQ diagnostics never count this path, so consumers see an apparently unconstrained request attribute even though extraction saw an enum constraint. Record these request-side failures in dropsByReason with the stanza and attribute occurrence.


if source_path.is_none()
&& let Some(value) = args.get(2).and_then(arg_expr).and_then(as_string_lit)

P2 Badge Surface nested literal-content pins

When literalContent validates a node reached through flattenedChildWithTag, source_path is non-empty, so this condition emits no content assertion. This arm always returns Binding::None and records no drop either, meaning a nested marker discriminator can disappear completely and the published shape accepts arbitrary content where WA pins one literal. Preserve the path with the assertion or report the constraint as unrepresentable.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-codegen/src/emit.rs Outdated
Comment thread crates/wa-scan/src/response_index.rs
Review round 20 — four findings. The first means the fields the last two rounds
"recovered" would have decoded to zero.

`contentUint` is not a text integer. WA calls it `contentUint(3)` for a prekey id,
`contentUint(4)` for a registration id, `contentUint(1)` for a type — the argument is a
BYTE COUNT and the body is big-endian binary. Grouping it with `contentInt` (which really
is decimal text: a follower count, a participant count) meant the generated parser ran
`content_str().parse()` over binary and silently produced `0` for every one, live in
`WAWebDigestKeyJob`. The byte count was being dropped on both scanner paths too, so the
IR did not even record the width; `byteLength` 1/3/4 now rides on those fields.

The other three:

* the multi-source refusal added last commit compared only the SECOND recovered accessor,
  so `combine(a("jid"), a("jid"), a("lid"))` matched on the second and never saw the
  conflicting third. Every argument is inspected now.
* the action enum-drop key was resolver-wide `wireName@method`, so two actions each
  losing `reason` counted once. Keyed by the table argument's source span, which is a
  stronger identity in both directions than the action name would be.
* the request-op fallback classified a candidate by its name suffix with an empty
  assertion list, so a `…Success…` module that asserts `type="error"` could be handed
  back as an operation's only response shape. The name gate stays as a cheap pre-filter;
  the parsed assertions decide.

538 tests, 12/12 schemas, determinism check clean.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 21815bdac2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-scan/src/response_index.rs
Comment thread crates/wa-scan/src/incoming.rs Outdated
Comment thread crates/wa-ir/src/wap.rs
Comment thread crates/wa-notif/src/actions.rs
Comment thread crates/wa-notif/src/actions.rs Outdated
…er carries

Review round 21 — five findings, three live in the committed artifacts.

`variant_kind` short-circuited only on `type="error"`, so a parser asserting
`type="result"` fell through to the name heuristic. `CreateResponseGroupAlreadyExists`
and `RequestSilentNonceResponseRecoveryRequired` both assert `result` and shipped as
`kind: "error"` — telling a consumer to route them opposite to the wire, in the one place
this change claims the discriminator outranks the name. The wire wins now; an error-ish
name on a `result` becomes `Alternative`, which is what that variant is for. `error` 47 →
45, `alternative` 0 → 2.

`attach_pending_enum_keys` hung `enumKeys` on the companion `maybeAttrString` read
without retyping it, so the incoming receipt's `type` shipped as `"string"` carrying nine
`enumKeys` — invisible to a consumer selecting enum fields by `type == "enum"`. It is
promoted to the enum accessor and type now.

The incoming ack pass ran its own smax `Resolver` and discarded its drops, so the ack's
`deprecatedEditMixin.edit` was an enum with no values while the domain reported nothing.
Merging them naively gave 29 — but that pass parses every `WASmaxIn*Response*` and keeps
only the `<ack>` roots, so 28 belonged to IQ modules absent from this artifact: the same
"the diagnostic does not describe the file beside it" bug fixed last round on the legacy
path. The collector is rewound for a module that contributes nothing, and the honest
number is 1, matching the one lost constraint the artifact actually carries.

Two more in the action reader: statements after an unconditional `return` were still
scanned, so unreachable returns became additional legal actions; and `{actionType:
"create"}` written as a literal resolved to `None`, publishing an arm with no identity.

542 tests, 12/12 schemas, determinism check clean.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2ec36a9989

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-scan/src/response_smax.rs Outdated
Comment thread crates/wa-scan/src/incoming.rs Outdated
Comment thread crates/wa-scan/src/response_smax.rs Outdated
Comment thread crates/wa-ir/src/wap.rs
…poisoning the cache

Review round 22 — four findings.

`normalize_accessor` rewrote `contentStringEnum` to `attrEnum`, so codegen — which
switches on the method to choose between an attribute and the element body — emitted
`get_attr(...)` for a value that is not an attribute. 12 fields in the committed IQ
artifact read that way. They are `contentEnum` now, keeping the enum type.

Last round's ack-drop rollback was not enough. It restored the diagnostic set but left
the resolver's caches populated, so a rejected non-ack module could record a loss on a
shared helper, have it rewound, and a later accepted ack would hit the cache and never
record it again. The ack pass builds a resolver PER CANDIDATE now (88 candidates, 8
kept) — a discarded resolver takes its cache with it, so no rejected work is observable
at all. The reported number is unchanged at 1.

`new Uint8Array(4)` is a length, and therefore exactly four zero bytes — as much a
compile-time constant as a literal array, and the request-side constant index already
models it that way. Refusing it lost a recoverable pin. Bounded, so a bogus length
cannot allocate.

`emit_field_parse` routed every JID through the unconditional `ok_or_else("missing …")`
branch while `rust_field_type` declares `Option<Jid>` for a `maybeAttr…Jid` — a `Jid`
initializer for an `Option<Jid>` field, which also rejects the absence the accessor
exists to permit. `union::field_expr` already had the optional branch; only this one
diverged.

That last one is *latent*, not live as reported: `iq.rs` has no `Option<Jid>` field today
and `notif.rs` emits no parser, so nothing miscompiles. It would the moment one appears —
and CI would not catch it, since the generated Rust is only syntax-checked. That is
follow-up 2 in the PR body, and this is the second finding in two rounds arguing for it.

545 tests, 12/12 schemas, determinism check clean.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5f10088c62

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-notif/src/actions.rs
Comment thread crates/wa-scan/src/enum_link.rs
Comment thread crates/wa-notif/src/actions.rs
Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-notif/src/actions.rs Outdated
Review round 23 — five findings, two of them live.

`fold_object` gives a module-local helper call first claim on its key. When the helper
only NORMALISES a value handed to it — `reason: hasAttr("reason") ? S(t.attrString(
"reason")) : null`, where `S` maps the string — its body reads no wire attribute of its
own, so inlining produced an empty shape and the key was dropped. `add`, `remove` and
`delete` lost their `reason` that way while `create`, whose wrapper is not a known local,
kept it. An empty inline now falls through to `read_field`, which finds the read in the
CALL's arguments and refuses the key outright if two arguments disagree. Shape elements
181 → 184.

The legacy enum-drop key was `site + table + attr` with no traversal path, so one parser
losing the same table for the same attribute under two children counted once. IQ's
`dropsByReason` was undercounting: 37 → 41. Every other per-occurrence key fixed in this
review was preventive; this one was hiding four real losses.

Three more, all latent:

* a `finally` was analyzed against the pre-`try` scope, so a name the `try` reassigned
  published its stale source. The body may also have thrown part-way, so neither binding
  is certain — every name `try`/`catch` writes is tombstoned for the finalizer.
* loop bodies were never traversed, so `while (c) { return A } return B` emitted only B.
* the mapped-child callback pre-installed the whole body's `var` initializers, making a
  later one visible to an earlier return — the exact thing `collect_returns` documents
  itself as avoiding. Removed; statement order already covers it, and the artifact does
  not move, so the pre-pass was contributing nothing but the bug.

548 tests, 12/12 schemas, determinism check clean.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cb533c8303

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-notif/src/actions.rs
Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-ir/src/notif.rs
Comment thread crates/wa-scan/src/response.rs
Review round 24 — five threads, four distinct findings.

`generated/notif/notif.rs` described the notification ENVELOPE and nothing inside it, so
a consumer using the advertised reference catalog instead of the JSON saw none of the 47
`w:gp2` arms this PR recovers. `NOTIF_ACTIONS` now carries every arm with its wire tag,
its normalized `actionType`, its fields, the constants it stamps and its mapped children,
plus an `actions_for(type)` lookup — emitted as data rather than 47 generated struct
names, since the arms are a many-to-one map with heterogeneous payloads. The
`not_ephemeral → ephemeral` + `duration: 0` normalization, the example the PR body leads
with, is now in the generated Rust. 512 → 605 lines.

`ModuleScopeBuilder` recorded enum tables at ANY function depth, first-wins, while the
helper branch beside it correctly required the factory body. Two parsers reusing one
minified local name would therefore share a table, and the second `u.members()` resolved
to the FIRST parser's values — a closed `enumKeys` set that is simply wrong. Invented
constraints are the worst outcome this change can produce, so the gate now matches the
helper branch. No artifact movement; the tables that resolve today are all module-scope.

Two more in the action reader, both latent:

* a bare block always runs and `var` is function-scoped, so `{ x = a("lid"); } return
  {id: x}` reads `lid` — analyzing it against a clone discarded the write. Blocks now
  hand their scope back; branch bodies still clone, deliberately.
* `if (c) return A; else return B; return C;` emitted C as a third legal action. The
  direct-`return` stop added last round is now the general "this statement exits on every
  path" check, which the existing analysis already knew how to answer.

551 tests, 12/12 schemas, determinism check clean.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

// FORM B carries a guard value in `value` that must be a member of the enum;
// FORM A leaves it `None`. Either way it's transient scan state, so clear it.
let guard = attr.value.take();

P2 Badge Preserve fixed values behind request enum guards

When a FORM B request attribute emits one fixed literal only while an enum guard matches, taking and clearing the guard value widens that attribute to the enum's entire value set. This is live for WAWebEncryptAndSendStatusMsg's optional session_scope: the builder emits only "status" when present, while the committed stanza IR advertises all SessionScope variants (default, status, and pq), allowing an emitter to construct values the builder never sends; preserve the literal restriction or narrow the enum reference to that member.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-codegen/src/notif_export.rs Outdated
Comment thread crates/wa-codegen/src/notif_export.rs
Comment thread crates/wa-notif/src/actions.rs
Comment thread crates/wa-scan/src/response_index.rs Outdated
Comment thread crates/wa-scan/src/response_index.rs
Comment thread crates/wa-notif/src/actions.rs
Comment thread README.md Outdated
Review round 25 — seven findings. Two are in the table added last round, and both
contradict a claim this change makes in its own README.

`field_list` emitted names, requiredness and a content flag, and dropped the field TYPE.
The committed artifact carries 24 `user_jid`, 7 `lid_user_jid`, 13 `group_jid` and 18
integers across the action arms — so the reference consumer had to treat a PN user JID
and a LID user JID as the same untyped string, which the README calls a
protocol-safety-critical distinction that must never collapse. A generated
`NotifFieldType` mirrors the IR vocabulary, with an exhaustive match so a new IR variant
fails to compile the generator rather than silently widening an enum consumers match on.

Constants were stringified into `(&str, &str)`, so `false` and the string `"false"`, `0`
and `"0"`, became indistinguishable in exactly the table whose purpose is to record what
the arm stamps. They are a typed `NotifConstValue` now.

`primary.is_empty()` was doing duty as "no success variant selected yet" — the same
emptiness-as-flag mistake the `finally` handling made two rounds ago. A fieldless first
success variant left it true, so a later one overwrote the primary shape and
`ParsedResponse.fields` contradicted the documented first-success rule. And the fallback
index rejected a parser for having no fields at all, though `makeResult({})` guarded by
`type="result"` is a confirmation whose assertions are the whole contract; a parser
carrying fields still wins, so this only adds a fallback.

The action-union switch is now identified by what it dispatches ON — a mapped child's tag
(`var a = t.tag(); switch (a)`) — instead of by having the most arms. A module holding an
unrelated const-keyed switch with more returning cases would otherwise have published its
constants as wire tags. A decoy fixture pins it.

Also: nested switch cases are analyzed with their fall-through suffix, like the outer one
already was; and the README's migration section said three changes when `ContentType`
gaining `integer` makes four.

553 tests, 12/12 schemas, determinism check clean.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review


P2 Badge Record unresolved attrEnumOrNull constraints

When attrEnumOrNullIfUnknown receives anything other than a recognized module-scope identifier—such as o("Mod").TABLE or a computed table—the lookup fails and this unconditional return bypasses field_from_call. The accessor consequently produces neither a pending enum link nor an unresolved-drop marker (and, without a companion read, no field at all), so the newly advertised diagnostics report no loss while the parser does enforce an enum constraint; fall through to the general enum accessor handling when the local-map fast path misses.


if source_path.is_none()
&& let Some(value) = args.get(2).and_then(arg_expr).and_then(as_string_lit)
{

P2 Badge Report unresolved literalContent discriminators

When literalContent pins the content through anything other than an inline string literal, this branch emits neither a Content assertion nor a drop diagnostic. A disjunction whose marker values are supplied through local constants or computed expressions can therefore publish fieldless variants with no usable discriminator while dropsByReason claims nothing was lost; record the unresolved occurrence when argument 2 cannot be resolved, just as the attribute-literal branches do.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-scan/src/response_index.rs
Comment thread crates/wa-notif/src/actions.rs Outdated
jlucaso1 added 2 commits July 28, 2026 10:19
…ck fix exposed

Review round 26 — four findings. One is a reproducibility hazard; the other three are
consequences of the block-scope propagation added two rounds ago.

`ResponseIndex::in_slices` was built by iterating a `HashMap`, and
`resolve_for_request_op` returns the FIRST matching candidate. An operation with two
success-like `WASmaxIn<X>Response*` modules would therefore attach a different response
parser — and commit different IR — from one run to the next. Byte reproducibility is this
repo's contract, so nothing feeding the artifact may depend on hash order; the candidates
are sorted by module name now. No op has two candidates today, which is the only reason
the determinism gate never flaked on it.

The other three, all in the action reader:

* the reassignment rule required the new value to resolve to a wire ACCESSOR, so the
  minifier's `var x; x = {actionType: A, id: attr("jid")}; return x` — a fully static
  shape — lost its binding and published an empty action. The name now points at whatever
  it holds, which serves the "never report an overwritten value" rule better than
  forgetting it: an uninterpretable value simply does not resolve into a field.
* a `do…while` body runs at least once, so its writes are definite. Grouping it with the
  zero-or-more loop forms tombstoned them.
* propagating a bare block's scope (round 24) leaked its `let`/`const` too, so an inner
  `let x` shadowed the outer `x` for the rest of the function when the runtime reads the
  outer one. `var` still escapes; block-scoped names are restored to what they shadowed.

556 tests, 12/12 schemas, determinism check clean.
Review round 27 — two findings, both a silent loss, which is the one outcome this change
exists to prevent.

`attrEnumOrNullIfUnknown` took a fast path for a module-scope identifier table and
returned unconditionally. Given anything else — `o("Mod").TABLE`, a computed set — it
produced no pending link, no drop, and with no companion read no field at all, while the
parser does enforce an enum there. The miss is now recorded and resolved alongside the
hits: an existing companion read is retyped to an enum carrying
`PendingEnum::Unresolvable`, and where no companion exists the field is synthesized, so
the constraint is counted either way instead of vanishing.

`literalContent` emitted a `Content` assertion only for an inline string literal. A
disjunction whose marker arrives through a local constant therefore published fieldless
variants with no usable discriminator while `dropsByReason` claimed nothing was lost. The
unresolvable occurrence is reported now, keyed per node path like its neighbours.

Both are latent against the pinned bundle — every table today is a module-scope
identifier and every marker an inline literal — so `generated/` and every drop count are
unchanged, and tests carry them.

Also worth noting for the review record: these two arrived in the review BODY rather than
as inline comments, so an unresolved-thread sweep does not see them. Both were verified
by reverting the fix and watching the test fail; the first attempt at that revert edited
the reason string in the test as well as the source, which made the test pass for the
wrong reason.

558 tests, 12/12 schemas, determinism check clean.
@jlucaso1

Copy link
Copy Markdown
Contributor Author

Rodada 27 — os dois achados do review 4797612204, aplicados em 2c09ff9.

Ambos eram perda silenciosa, que é exatamente o desfecho que esta mudança existe para eliminar:

  • attrEnumOrNullIfUnknown tinha um fast path para tabela identificada por nome de módulo e retornava incondicionalmente. Com qualquer outra coisa (o("Mod").TABLE, conjunto computado) não produzia link, nem drop, nem — sem read companheiro — campo algum, enquanto o parser enforça um enum ali. Agora a falha é registrada e resolvida junto com os acertos: read companheiro existente é retipado para enum com PendingEnum::Unresolvable, e sem companheiro o campo é sintetizado.
  • literalContent só emitia assertion para literal inline. Uma disjunção cujo marcador vem de constante local publicava variantes sem discriminador usável enquanto dropsByReason afirmava que nada se perdeu. Agora a ocorrência é reportada, com chave por caminho de nó como as vizinhas.

Ambos latentes no bundle fixado — hoje toda tabela é identificador de módulo e todo marcador é literal inline — então generated/ e todas as contagens de drop ficam inalteradas, e os testes é que os sustentam.

Duas notas de processo que valem mais que os achados:

  1. Estes vieram no corpo do review, não como comentários inline. Minha varredura de threads não-resolvidas não os enxerga — foi você que me mandou o link. Vou passar a varrer os corpos de review também.
  2. Verifiquei cada um revertendo a correção e vendo o teste falhar. Na primeira tentativa do literalContent, meu revert trocou a string da razão no teste também, então o teste passou pelo motivo errado. Refiz mexendo só na produção; aí falhou com left: None, right: Some(1), como deve.

558 testes, 12/12 schemas, clippy limpo, determinismo em dia.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

&& b.assertions
.iter()
.any(|y| y.kind == AssertionKind::Attr && y.name == x.name && y.value != x.value)

P2 Badge Require literals on both sides of assertion conflicts

When one variant pins an attribute and another merely asserts that the same attribute exists, this treats Some("result") != None as a conflicting discriminator. The variants are not actually disjoint—the presence-only parser also accepts type="result"—so the separability gate may generate a first-success outcome parser whose earlier arm shadows the later one; require both assertions to carry distinct literal values (and concrete matching names) before declaring a conflict.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-scan/src/response_index.rs
Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-scan/src/response.rs
Comment thread crates/wa-codegen/src/notif_export.rs Outdated
…ructure

Review round 28 — five findings: four inline, one in the review body (the monitor now
surfaces those, which is how the fifth was seen at all).

Two published constraints that are not true, the worst outcome this change can produce:

* `obj_props` silently skips a spread, a method and a computed key, so a module-local
  enum containing one collected its SURVIVING literals successfully and shipped them as a
  closed set — claiming the parser rejects whatever the skipped property contributed. A
  table that cannot be read whole is not a table; it is refused and counted.
* `assertions_conflict` read `Some("result") != None` as a discriminator conflict. A
  parser that merely requires `type` to exist also accepts `type="result"`, so the two
  variants are not disjoint, and the separability gate would emit a first-success cascade
  whose earlier arm shadows the later. Both sides must now pin a literal on the same
  named attribute.

The fieldless-parser fix from round 25 picked the wrong export when a module has both:
preferring "whichever has fields" chose the payload helper over a `makeResult({})` root,
losing the root's assertions — which for a confirmation ARE the contract. Selection is
structural now (a child export's name extends another's at a word boundary), the same
rule the incoming ack scan uses.

Also: a `catch` parameter shadows the outer binding, so `var e = attrString("jid");
catch (e) { return {id: e} }` published `id` as a read of `jid`; and the generated
`NotifActionField` now carries `enum_values`, since `NotifFieldType::Enum` says a value
is constrained but not to what. No action field resolves an enum today, so that one is
latent.

561 tests, 12/12 schemas, determinism check clean.

Each fix was verified by reverting it alone and watching its test fail. The first attempt
reverted all three in one script, which aborted on the first mismatched anchor and left
every fix in place — the tests then passed for the wrong reason. Third time that has
happened in this review; one revert per run, checked.
@jlucaso1

Copy link
Copy Markdown
Contributor Author

Rodada 28 — cinco achados, aplicados em 25ad520. Quatro inline e um no corpo do review (spec.rs), que só enxerguei porque corrigi o monitor nesta sessão.

Duas restrições inventadas — a categoria pior:

  • obj_props pula spread/método/chave computada em silêncio, então o collect::<Option<Vec<_>>>() que adicionei na rodada 20 justamente para exigir tudo-ou-nada tinha sucesso sobre os literais sobreviventes e publicava conjunto fechado. Agora exijo properties.len() == obj_props().count().
  • assertions_conflict lia Some("result") != None como conflito. Um parser que só exige type existir também aceita type="result" — as variantes não são disjuntas, e o gate de separabilidade emitiria uma cascata cujo arm anterior sombreia o posterior.

Uma correção minha da rodada 25 escolhia o export errado. "Quem tem campos ganha" pega o helper de payload sempre que a raiz é confirmação sem campos, perdendo as assertions da raiz — que numa makeResult({}) são o contrato inteiro. Agora a seleção é estrutural, reusando a regra que o scan de ack do incoming já tinha.

Mais: parâmetro de catch sombreia o binding externo; e NotifActionField ganhou enum_values, porque NotifFieldType::Enum diz que o valor é restrito mas não a quê (latente — nenhum campo de ação resolve enum hoje).

561 testes, 12/12 schemas, determinismo e clippy limpos.

Nota de processo, terceira ocorrência. Verifiquei cada correção revertendo-a e vendo o teste falhar. Na primeira tentativa revertí as três num script só; ele abortou na primeira âncora que não casou (o cargo fmt tinha reformatado a linha) e nenhum revert aplicou — os três testes passaram pelo motivo errado. Refiz um revert por execução, com asserção explícita. É a terceira vez nesta revisão que essa verificação precisou de segunda tentativa, sempre pela mesma causa: substituição de texto casando com o que eu não pretendia (ou não casando nada).

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 25ad520b05

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-codegen/src/emit.rs
Comment thread crates/wa-notif/src/actions.rs
Comment thread crates/whatspec/src/main.rs
Comment thread crates/wa-scan/src/response_smax.rs Outdated
Review round 29 — four findings. The first is live at scale and rejects valid responses.

`rust_field_type` and `emit_field_parse` derived optionality from the accessor SPELLING
alone, but a smax tail conditional makes a field optional without renaming its accessor.
The committed artifact carries **108** JID fields that are `attrUserJid`/`attrLidUserJid`/…
with `required: false`; every one was declared a plain `Jid` and parsed with
`ok_or_else("missing …")?`, so the generated parser rejected responses the IR explicitly
says may omit them. `required` is now a second source of optionality, in the struct type
and the parser alike (`union::field_expr` too). `pub to: Jid` → `pub to: Option<Jid>`, and
`missing to` disappears from the generated file.

The `errorTexts` floor counted DISTINCT text strings, so one RPC losing its `bad-request`
pin while another still carried it moved nothing — and the arm survives, so `errorArms`
did not move either. The guard could pass while a specific RPC's accepted `(code, text)`
pairing silently degraded. Keyed by `(module, variant, arm, text)` now: 38 → 577, and the
doc says what the number means.

`stmt_exits` had no `do…while` arm, so `do { return A } while (c); return B` published the
unreachable B; and an unresolvable `contentUint` width — a hoisted constant hides the
number, not the constraint — produced an unrestricted integer with no drop recorded.

562 tests, 12/12 schemas, determinism check clean. Each fix reverted alone to watch its
test fail; two of the four edits in this round again failed to apply as text substitutions
before being redone against the formatted source.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 37698d926f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-notif/src/actions.rs
Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-notif/src/actions.rs
Comment thread crates/whatspec/src/main.rs Outdated
jlucaso1 added 2 commits July 28, 2026 11:42
…s inventing

Review round 30 — six findings, two live.

`fieldEnumRefs` counted only a resolved `enumRef`, ignoring `enumKeys` — the same closed
allowed-value constraint, resolved in-module instead of cross-module. One such field is
live (`type` accepting only `error`/`result`), so losing that table would have left the
floor unmoved. 150 → 151.

`string_const_object` accepted a partial map when the object held a spread: `obj_props`
skips it, so `{ADD: "old", ...base}` resolved `ADD` to `old` while the runtime switch uses
the spread's value, and a spread-only member vanished. Either way the arm gets a wireTag
or actionType the handler never produces. Same rule the response-side enum tables got two
rounds ago — a table that cannot be read whole is not a table.

Three more shapes the reader was asserting rather than reading:

* `c ? attrString("jid") : attrString("jid")` — the same read on both paths runs on every
  execution, but falling through to the plain path made `read_field` see a conditional and
  publish `required: false` for an accessor that still rejects an absent value.
* `{meta: {id: …}}` in a mapped-child callback had its inner `id` hoisted into the child's
  flat fields, claiming a top-level attribute the element does not have.
* a `do…while` whose body can `break` is not a definite write; the break path reaches the
  return with the binding unset.

And the helper-parameter binding I declined in round 23 is implemented: a helper is now
applied to its call-site argument TEXT (re-parsed in the same allocator, which is what
makes the binding expressible), so `function mk(v){ return {id: v} }` called as
`mk(t.attrString("jid"))` keeps `id`. Applied ONLY when an argument is itself a wire read
— binding the node-passing form's formals to identifiers absent from this parse resolved
them to nothing and cost 62 shape elements before I caught it.

566 tests, 12/12 schemas, determinism check clean.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 93ae213942

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-notif/src/actions.rs Outdated
Comment thread crates/wa-notif/src/actions.rs
Comment thread crates/wa-notif/src/actions.rs
Comment thread crates/wa-notif/src/actions.rs
Comment thread crates/wa-notif/src/actions.rs
Comment thread crates/wa-notif/src/actions.rs
Comment thread crates/wa-notif/src/actions.rs
Comment thread crates/wa-notif/src/actions.rs
Review round 31 — eight findings, five applied, three deferred with evidence (below).

* a `break` consumed by a `do…while` cannot prove the loop exits its CALLER, so
  `do { break; } while (c); return B` stopped before the reachable B.
* a `finally` runs on every path, so its writes are definite; tombstoning the whole `try`
  statement removed them along with the uncertain ones.
* a `for` INITIALIZER runs before every path into the body, so the body must be analyzed
  against it, not against the pre-loop scope.
* a write buried in a short-circuit or ternary (`c && (x = a("lid"))`) runs on some paths
  only; ignoring it left the previous binding asserted as fact.
* a returned action object containing a spread cannot be read whole, so it is refused —
  the same rule the constant tables and enum tables already follow.

571 tests; each fix reverted alone to watch its test fail. Artifact unchanged.

DEFERRED, and the reason is worth recording. `generated/notif/index.json` has not moved
since cb533c8 — eight commits and six rounds of work in this file. Every finding since
has been a JavaScript construct the hand-rolled dataflow analysis in `actions.rs` does not
model, and three of the eight this round (`do…while` at all, an object spread, a
by-name mapped-child callback) do not occur anywhere in the pinned handler module. The
three left out — resolving an accessor ALIAS passed to a helper, resolving a named
callback through the local index, and marking a branch-only child collection optional —
are real and each needs new plumbing or an IR field.

That is a poor trade at this point, and not only on cost: patching this analysis
construct-by-construct has itself introduced regressions in rounds 19, 23, 24, 26 and 30,
including one in this very round's predecessor that cost 62 shape elements before a
counter caught it. The class wants either a documented scope boundary or a real dataflow
pass, not another round of constructs.
@jlucaso1
jlucaso1 merged commit 96a69fb into main Jul 28, 2026
4 checks passed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bca400b757

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/wa-ir/src/lib.rs
Comment on lines +79 to +80
/// The migration is one variant wide: handle (or ignore) `kind: "reference"` on response
/// assertions. Everything else in 2.0.0 is opt-in.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Document all breaking 2.0 migrations

When a 1.x consumer follows this Rustdoc, handling only AssertionKind::Reference is insufficient: this commit also adds the closed-enum values ResponseVariantKind::{ClientError, ServerError} and ContentType::Integer, so exhaustive Rust matches and schema-generated consumers still fail. The corrected migration list in README.md:34-40 already identifies these changes, so update this crate-level guidance instead of claiming every other 2.0 change is opt-in.

Useful? React with 👍 / 👎.

Comment on lines +126 to +129
|a| {
let sp = oxc_span::GetSpan::span(a);
format!("{}..{}@{method}", sp.start, sp.end)
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Include handler identity in enum-drop keys

When two different notification handler modules have unresolved enum arguments at the same module-local source offsets, these keys collide because each handler_slice is parsed separately and its spans restart from zero, while ConstResolver::enum_drops is shared across all handlers. One loss is then omitted from diagnostics.notif.dropsByReason, allowing the enum-constraint coverage guard to miss a regression; include the handler/module identity along with the span.

Useful? React with 👍 / 👎.

if self.def.action_type.is_none() {
self.def.action_type = from.action_type;
}
merge_fields(&mut self.def.fields, from.fields, false, &mut self.dead);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve constant-vs-wire alternatives when folding branches

When branches with the same wireTag and actionType bind one output key differently—for example, one returns {duration: 0} while another returns {duration: child.attrInt("expiration")}—the field merge adds an optional wire field and the later constant intersection discards the constant. The published action then claims the constant branch omits duration, although it always produces 0, losing precisely the normalization that constantFields is intended to expose; treat cross-kind bindings as alternatives or keep the branches separate.

Useful? React with 👍 / 👎.

Comment on lines 349 to +353
// Tag (the `<iq>` root) / FromServer are not success-vs-error discriminators.
AssertionKind::Tag | AssertionKind::FromServer => {}
// Neither is a Reference echo (`from` == the request's `to`): every outcome
// of the same request satisfies it identically, so it separates nothing —
// it is a request-correlation rule, enforced by whoever holds the request.
AssertionKind::Tag | AssertionKind::FromServer | AssertionKind::Reference => {}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Enforce reference assertions in generated response parsers

When a response has the newly recovered echo constraints (id equal to the request's id, or from equal to its to), the generated IqSpec::parse_response discards them here and emits neither a check nor metadata through which its caller can apply one. A correctly shaped response carrying another request's id or target therefore passes the reference parser despite violating the parser contract represented in the IR; emit a common request-correlation guard outside the success/error variant dispatch.

Useful? React with 👍 / 👎.

Comment on lines +946 to +949
collect_returns(&[&i.consequent], scope, out);
if let Some(alt) = &i.alternate {
collect_returns(&[alt], scope, out);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Apply condition assignments before scanning branch returns

When an if test itself rebinds a tracked local, such as var x = child.attrString("jid"); if (x = child.attrString("lid")) return {id: x};, the assignment executes before the consequent, but the consequent is scanned with the old scope and publishes jid. The later tombstone only affects statements after the if, so it cannot repair the already emitted wrong branch shape; evaluate definite test-expression writes into the branch-entry scope before collecting either arm.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant